Multiperson chat server using the select() API

Question

I followed the Beej's Guide to Network Programming. The program acts like a multi-user chat server. One connects and sends a message to the server, which is forwarded to all that are connected. There's a simple makefile below; also Valgrind output.

Review Goals:

I have specific questions:

• Is my code POSIX compliant?
• Do you see any outdated practices or functions?
• Does any part of my code invoke undefined behaviour?
• Is it portable (across UNIX-like systems)?
• How much more information can I glean about a client from a given file descriptor?
• Is there an alternative to errno/perror style of error checking?
• Currently, I'm only logging the client's host and service name. What other sort of information should I log? Should I look into syslog?
• Do you see any potential buffer overflows?
• How can I improve the code?

Code:

log.h:

#ifndef LOG_H
#define LOG_H

#define LOG_TIME            0x01        /* 0b00000001 */
#define LOG_DATE            0x02        /* 0b00000010 */
#define LOG_USER            0x04        /* 0b00000100 */
#define LOG_COUNT           0x08        /* 0b00001000 */
#define LOG_ALL             0xFF        /* 0xFFFFFFFF */
#define LOG_FULLTIME        0x03    /* 0x00000011 */

#define TS_BUF_LENGHT       50

void log_msg (FILE * fp, const char *msg, uint8_t options);

#endif

log.c:

void log_msg (FILE * fp, const char *msg, uint8_t options)
{
    time_t time_val;
    static long long log_count = 0;
    char time_stamp[TS_BUF_LENGHT];
    char date_stamp[TS_BUF_LENGHT];
    struct tm *tm_info;

    time_val = time (0);
    tm_info = localtime (&time_val);

    (void) strftime (date_stamp, TS_BUF_LENGHT, "%F (%a)", tm_info);
    (void) strftime (time_stamp, TS_BUF_LENGHT, "%H:%M:%S", tm_info);

    if (options & LOG_COUNT) {
        fprintf (fp, "%lld\n, ", ++log_count);
    }
    if (options & LOG_DATE) {
        fprintf (fp, "%s, ", date_stamp);
    }
    if (options & LOG_TIME) {
        fprintf (fp, "%s, ", time_stamp);
    }

    fprintf (fp, "%s\n", msg);
}

selectserver.h:

#ifndef SELECTSERVER_H
#define SELECTSERVER_H

#define _POSIX_C_SOURCE 200809

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
#include <errno.h>
#include <signal.h>

#include <unistd.h>
#include <sys/types.h>
#include <sys/time.h>
#include <netdb.h>
#include <sys/socket.h>
#include <netinet/tcp.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <sys/ioctl.h>

#define PROGRAM_NAME "selectserver"
#define PORT "9808"             /* Port we are listening on */
#define MAX_LOG_TEXT 2048       /* Max text lenght for logging */

#ifdef BUFSIZ                   /* Max text lenght */
#define BUFSIZE BUFSIZ
#else
#define BUFSIZE 4096
#endif

#define ARRAY_CARDINALITY(x) (sizeof(x) / sizeof (x[0]))
#define NI_MAXHOST 1024
#define NI_MAXSERV 35
#define FFLUSH(fp) if (fflush (fp) == EOF) perror ("fflush()")
#define FSYNC(fd)  if (fsync (fd) == -1) perror ("fsync()")

#endif

selectserver.c:

*/
*  selectserver.c -- a multiperson chat server
*/

#include "selectserver.h"
#include "log.h"

FILE *log_fp = 0;
int log_fd = 0;

static void exit_cleanup (void)
{
    if (log_fp) {
        (void) fclose (log_fp);
    }
}

static void sigint_handler (int sig)
{
    (void) close (log_fd);
    (void) signal (sig, SIG_DFL);
    (void) raise (sig);
}

static int init_addr (struct addrinfo **servinfo)
{
    struct addrinfo hints;

    memset (&hints, 0x00, sizeof hints);
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_flags = AI_PASSIVE;

    int status = 0;
    errno = 0;
    if ((status = getaddrinfo (0, PORT, &hints, servinfo)) != 0) {
        perror ("getaddrinfo()");
        return -1;
    }
    return 0;
}

static int init_sock (struct addrinfo **servinfo)
{
    int master_fd = 0;
    struct addrinfo *p = 0;
    int yes = 1;
    socklen_t size = sizeof (yes);
    errno = 0;

    for (p = *servinfo; p; p = p->ai_next) {
        master_fd = socket (p->ai_family, p->ai_socktype, p->ai_protocol);
        if (master_fd == -1) {
            perror ("socket()");
            continue;
        }
        errno = 0;
        if (setsockopt
            (master_fd, SOL_SOCKET, SO_REUSEADDR, (void *) &yes,
             size) == -1) {
            perror ("setsockopt()");
            continue;
        }
        errno = 0;
        if (bind (master_fd, p->ai_addr, p->ai_addrlen) == -1) {
            close (master_fd);
            perror ("bind");
            continue;
        }
        break;
    }

    if (!p) {
        (void) fprintf (stderr, "%s: Failed to setup a socket.\n",
                        PROGRAM_NAME);
        return -1;
    }

    errno = 0;
    if (listen (master_fd, SOMAXCONN) == -1) {
        perror ("listen()");
        return -1;
    }
    return master_fd;
}

/* Synopsis: Writes the host and service names to stderr and log file.
*/
static void write_slave_info (int slave_fd, struct sockaddr slave_addr,
                              socklen_t addr_len)
{
    char log_txt[MAX_LOG_TEXT] = { 0 };
    char host[NI_MAXHOST] = { 0 };
    char service[NI_MAXSERV] = { 0 };
    int ret_val = 0;
    errno = 0;

    if ((ret_val =
         getnameinfo (&slave_addr, addr_len, host, sizeof host, service,
                      sizeof service, NI_NUMERICHOST)) != 0) {
        (void) fprintf (stderr, "%s: getnameinfo: %s\n", PROGRAM_NAME,
                        gai_strerror (ret_val));
    }

    (void) snprintf (log_txt, sizeof log_txt,
                     "INFO: New connection from HOST:%s, SERVICE:%s, on socket %d.",
                     host, service, slave_fd);
    log_msg (stderr, log_txt, LOG_FULLTIME);
    log_msg (log_fp, log_txt, LOG_FULLTIME);

    errno = 0;
    FFLUSH (log_fp);

    errno = 0;
    FSYNC (log_fd);
}

/*  Returns:  The slave file descriptor on success, -1 otherwise.
*/
static int accept_new_connection (int master_fd)
{
    int slave_fd = 0;
    struct sockaddr slave_addr = { 0 };
    socklen_t addr_len = sizeof slave_addr;
    int yes = 1;
    socklen_t size_yes = sizeof yes;
    int flag = 5;
    socklen_t size_flag = sizeof flag;
    errno = 0;

    do {
        errno = 0;
        slave_fd = accept (master_fd, &slave_addr, &addr_len);
    } while ((slave_fd == -1) && (errno == EINTR));

    if (slave_fd == -1) {
        perror ("accept()");
        return -1;
    }

    errno = 0;
    if (setsockopt
        (slave_fd, SOL_SOCKET, SO_KEEPALIVE, (void *) &yes,
         size_yes) == -1) {
        perror ("setsockopt()");
        return -1;
    }
    errno = 0;
    if (setsockopt
        (slave_fd, IPPROTO_TCP, TCP_KEEPCNT, (void *) &flag,
         size_flag) == -1) {
        perror ("setsockopt()");
        return -1;
    }
    errno = 0;
    if (setsockopt
        (slave_fd, IPPROTO_TCP, TCP_KEEPINTVL, (void *) &flag,
         size_flag) == -1) {
        perror ("setsockopt()");
        return -1;
    }

    write_slave_info (slave_fd, slave_addr, addr_len);
    return slave_fd;
}

static void write_farewell (int slave_fd)
{
    char log_txt[MAX_LOG_TEXT] = { 0 };
    (void) snprintf (log_txt, sizeof log_txt,
                     "INFO: Socket %d hung up.\n", slave_fd);
    log_msg (stderr, log_txt, LOG_FULLTIME);
    log_msg (log_fp, log_txt, LOG_FULLTIME);
    FFLUSH (log_fp);
    FSYNC (log_fd);
}

/* Synopsis: Get the number of bytes that are immediately available for
*            reading.
*  Returns:  -1 on failure, number of bytes available elsewise.
*/
static int get_bytes (int slave_fd)
{
    int flag = 0;
    errno = 0;

    if (ioctl (slave_fd, FIONREAD, &flag) == -1) {
        perror ("ioctl()");
        return -1;
    }
    return flag;
}

/* Synopsis: Calls recv() in a loop to read as much as available.
*
*  Returns:  0 on allocation failure, storing 0 in status, a pointer
*            to the line elsewise.
*            Stores -1 in status in case of a closed connection
*            or a recv() error.
*
*  Remarks:  The caller is responsible for freeing the returned
*                        memory (in case of success), else we risk exhaustion.
*/
static char *read_line (size_t *nbytes, int slave_fd, int *status)
{
    static const size_t page_size = BUFSIZE;
    char *buf = realloc (0, page_size);
    if (!buf) {
        errno = ENOMEM;
        perror ("realloc()");
        free (buf);
        status = 0;
        return 0;
    }
    int flag = 0;
    ssize_t ret_val = 0;
    size_t total = 0;

    do {
        ret_val = recv (slave_fd, buf + ret_val, page_size - 1, 0);
        if (ret_val > 0) {
            total += (size_t) ret_val;
            buf[ret_val] = '\0';

            flag = get_bytes (slave_fd);
            if (flag == -1) {
                free (buf);
                *status = -1;
                return 0;
            }
            if (flag > 0) {
                if (flag > ((page_size - 1) - (size_t) ret_val)) {
                    char *new = realloc (buf, page_size + page_size);
                    if (!new) {
                        errno = ENOMEM;
                        perror ("realloc()");
                        free (buf);
                        status = 0;
                        return 0;
                    }
                }
            }
        } else {
            flag = 0;
        }
    } while (flag > 0);

    /* Slave closed the connection
     */
    if (ret_val == 0) {
        write_farewell (slave_fd);
        free (buf);
        *status = -1;
        return 0;
    }
    /* We have an error, sire!
     */
    if (ret_val == -1) {
        perror ("recv()");
        free (buf);
        *status = -1;

    }
    *nbytes = total;
    return buf;

}

/* Synopsis: Calls send() in a loop to ensure that all data is sent.
                         Stores the number of bytes sent in len.
*  Returns:  0 on success, -1 otherwise.
*/
static int send_all (int slave_fd, const char *line, size_t *len)
{
    size_t total = 0;
    size_t bytes_left = *len;
    ssize_t status = 0;

    while (total < *len) {
        errno = 0;
        status = send (slave_fd, line + total, bytes_left, 0);
        if (status == -1) {     /* A send() error */
            if (errno == EINTR) {
                continue;
            }
            perror ("send()");
            break;
        }
        total += (size_t) status;
        bytes_left -= (size_t) status;
    }
    *len = total;
    return status == -1 ? -1 : 0;
}

static void send_msg (size_t nbytes, const char *line, int sender_fd,
                      int master_fd, fd_set master, int fd_max)
{
    for (int i = 0; i <= fd_max; i++) {
        /* Send it to everyone */
        if (FD_ISSET (i, &master)) {
            /* Excluding the master and sender */
            if (i != master_fd && i != sender_fd) {
                size_t len = nbytes;
                int status = send_all (i, line, &len);
                if (status == -1) {
                    perror ("send()");
                } else if (len != nbytes) {
                    (void) fprintf (stderr,
                                    "%s: We only sent %ld bytes because of a send() error.\n",
                                    PROGRAM_NAME, len);
                }
            }
        }
    }
}

/* Synopsis: Calls select and handles new connections.
*  Returns:  -1 on failure to init select or ENOMEM, 0 otherwise.
*/
static int handle_connections (int master_fd)
{
    fd_set master;              /* Master file descriptor list */
    fd_set read_fds;            /* Temp file descriptor list for select() */
    int fd_max = 0;             /* Max descriptor seen so far */

    FD_ZERO (&master);
    FD_ZERO (&read_fds);

    FD_SET (master_fd, &master);

    fd_max = master_fd;

    for (;;) {
        errno = 0;
        /* Because select is destructive */
        read_fds = master;
        if (select (fd_max + 1, &read_fds, 0, 0, 0) == -1) {
            if (errno == EINTR) {
                continue;
            }
            perror ("select()");
            return -1;
        }

        /* Iterate through the existing connections looking for data to read */
        for (int i = 0; i <= fd_max; i++) {
            /* We have a connection */
            if (FD_ISSET (i, &read_fds)) {
                /* It's the master */
                if (i == master_fd) {
                    int slave_fd = 0;
                    if ((slave_fd =
                         accept_new_connection (master_fd)) != -1) {
                        FD_SET (slave_fd, &master);
                    }
                    if (slave_fd > fd_max) {
                        fd_max = slave_fd;
                    }
                }               /* We have data to read */
                else {
                    size_t nbytes = 0;
                    int status = 0;
                    char *line = read_line (&nbytes, i, &status);
                    /* A read error, or the slave closed connection */
                    if (!line && !status) {
                        return -1;      /* ENOMEM */
                    } else if (status == -1) {
                        FD_CLR (i, &master);
                        close (i);
                    } else {
                        send_msg (nbytes, line, i, master_fd, master,
                                  fd_max);
                        free (line);
                    }
                }
            }
        }
    }
    return 0;
}

#define ARRAY_CARDINALITY(x) (sizeof(x) / sizeof (x[0]))

int main (void)
{
    static sigset_t caught_signals;
    sigemptyset (&caught_signals);

    static int const sig[] = {
        SIGALRM, SIGHUP, SIGINT,
        SIGPIPE, SIGQUIT, SIGTERM,
    };

    const size_t nsigs = ARRAY_CARDINALITY (sig);

    struct sigaction act;

    for (size_t i = 0; i < nsigs; i++) {
        if (sigaction (sig[i], 0, &act) == -1) {
            perror ("sigaction()");
            return EXIT_FAILURE;
        }

        if (act.sa_handler != SIG_IGN) {
            sigaddset (&caught_signals, sig[i]);
        }
    }
    act.sa_handler = sigint_handler;
    act.sa_mask = caught_signals;
    act.sa_flags = 0;

    for (size_t i = 0; i < nsigs; i++) {
        if (sigismember (&caught_signals, sig[i])) {
            if (sigaction (sig[i], &act, 0) == -1) {
                perror ("sigaction()");
                return EXIT_FAILURE;
            }
        }
    }
    atexit (exit_cleanup);

    errno = 0;
    log_fp = fopen ("server.log", "a");
    if (!log_fp) {
        perror ("fopen()");
        return EXIT_FAILURE;
    }
    errno = 0;
    log_fd = fileno (log_fp);
    if (log_fd == -1) {
        perror ("fileno()");
        return EXIT_FAILURE;
    }
    int master_fd = 0;          /* One descriptor to rule them all */
    struct addrinfo *servinfo;

    /* Set up the server address */
    if (init_addr (&servinfo) == -1) {
        return EXIT_FAILURE;
    }
    /* Open a socket and set it to listening */
    if ((master_fd = init_sock (&servinfo)) == -1) {
        freeaddrinfo (servinfo);
        return EXIT_FAILURE;
    }
    freeaddrinfo (servinfo);

    /* Wait for and eventually handle a new connection */
    (void) printf ("\nListening for connections on port %s.\n", PORT);

    if (handle_connections (master_fd) == -1) {
        close (master_fd);
        return EXIT_FAILURE;
    }
    close (master_fd);
    return EXIT_SUCCESS;
}

Formatting:

indent -kr -cs -pcs -saf -sai -saw --no-tabs --case-indentation 4 selectserver.c selectserver.h log.h log.c

Makefile:

# the compiler:
CC = gcc-10

# compiler flags:
CFLAGS = -O2 -pedantic-errors -Wall -Wfloat-equal -Wundef \
         -Wshadow -Wpointer-arith -Wcast-align -Wstrict-prototypes \
         -Wstrict-overflow=5 -Wconversion -Wformat-overflow=2 \
         -Wcast-qual -Wformat-signedness -Wformat-truncation=1\
         -Wunreachable-code -fanalyzer -Wformat=2 -fno-builtin \
         -std=c17 -Winit-self -Wmaybe-uninitialized \
         -Wuninitialized -Werror=implicit-function-declaration \
         -Wmisleading-indentation -Wmissing-braces \
         -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls \
         -Wundef -fno-omit-frame-pointer -fno-common \
         -fsanitize=address -fsanitize=undefined

TARGET = selectserver1.1
OBJS = selectserver.o log.o
DEPS = log.h selectserver.h

$(TARGET): $(OBJS)
        $(CC) -o $@ $^ $(CFLAGS)

%.o: %.c $(DEPS)
        $(CC) -c -o $@ $< $(CFLAGS)

clean:
        rm -rf *.o

Dynamic analysis:

/* Valgrind's dump after I sent 100 simultaneous connections with a message to the server. 
*  I exited with SIGINT (or Ctrl-C).
*
*  Flags passed: valgrind --leak-check=full --show-leak-kinds=all --show-error-list=yes -s
*                                         --leak-check-heuristics=all --log-file=dynamic_analysis  ./selectserver1.1
*/

==1466== Memcheck, a memory error detector
==1466== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==1466== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==1466== Command: ./selectserver1.1
==1466== Parent PID: 8
==1466==
==1466== error calling PR_SET_PTRACER, vgdb might block
==1466==
==1466== Process terminating with default action of signal 2 (SIGINT)
==1466==    at 0x497FF7A: select (select.c:41)
==1466==    by 0x10A0A4: handle_connections (selectserver.c:326)
==1466==    by 0x10A565: main (selectserver.c:440)
==1466==
==1466== HEAP SUMMARY:
==1466==     in use at exit: 472 bytes in 1 blocks
==1466==   total heap usage: 575 allocs, 574 frees, 1,352,203 bytes allocated
==1466==
==1466== 472 bytes in 1 blocks are still reachable in loss record 1 of 1
==1466==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1466==    by 0x48ED92D: __fopen_internal (iofopen.c:65)
==1466==    by 0x48ED92D: fopen@@GLIBC_2.2.5 (iofopen.c:86)
==1466==    by 0x10A4DC: main (selectserver.c:412)
==1466==
==1466== LEAK SUMMARY:
==1466==    definitely lost: 0 bytes in 0 blocks
==1466==    indirectly lost: 0 bytes in 0 blocks
==1466==      possibly lost: 0 bytes in 0 blocks
==1466==    still reachable: 472 bytes in 1 blocks
==1466==         suppressed: 0 bytes in 0 blocks
==1466==
==1466== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Answer 1

log.h

I don't think that calling code has any need for TS_BUF_LENGHT, so this could be moved to the implementation file. Consider spelling LENGTH the usual way, too.

FILE and uint8_t are undefined. You need to include <stdio.h> and <stdint.h>, as it's not possible to portably forward-declare these typedefs.

Note that uint8_t is provided only when the underlying hardware has support for 8-bit values without padding. There's no good reason to restrict the number of flags so tightly this early in development - I recommend using plain unsigned int there.

log.c

We're missing declarations from <time.h> and <stdio.h>.

Don't ignore the return value: if we're running in a locale where the output exceeds the provided maximum length, then

strftime() returns 0, and the contents of the array are undefined.

(my emphasis). So check the result before using the buffer. We can reduce the work in this function by calling the time-related functions only when we're actually going to use the output:

void log_msg (FILE * fp, const char *msg, unsigned options)
{
    time_t time_val = options & LOG_FULLTIME ? time (0) : 0;
    static long long log_count = 0;
    char time_stamp[TS_BUF_LENGTH];
    char date_stamp[TS_BUF_LENGTH];
    struct tm *tm_info = options & LOG_FULLTIME ? localtime (&time_val) : NULL;

    if (options & LOG_COUNT) {
        fprintf (fp, "%lld\n, ", ++log_count);
    }
    if (options & LOG_DATE && strftime (date_stamp, TS_BUF_LENGTH, "%F (%a)", tm_info)) {
        fprintf (fp, "%s, ", date_stamp);
    }
    if (options & LOG_TIME && strftime (time_stamp, TS_BUF_LENGTH, "%H:%M:%S", tm_info)) {
        fprintf (fp, "%s, ", time_stamp);
    }

    fprintf (fp, "%s\n", msg);
}

If this program is to run as a daemon, it's probably better to allow use of syslog() - that takes care of re-opening the file during log rotation, for example.

selectserver.h

That's a lot of includes for a header file! None of them seem to be necessary (though it's polite to include <stdio.h> so the macros can be expanded).

However, I don't think we need this header at all - the implementation is where main() is, so these definitions can simply all go into selectserver.c.

POSIX_C_SOURCE should expand to a long constant, so it's best to define it as 200809L.

ARRAY_CARDINALITY() improperly protects its argument from precedence errors - we need (x)[0] rather than (x[0]):

#define ARRAY_CARDINALITY(x) (sizeof (x) / sizeof (x)[0])

The FFLUSH() and FSYNC() macros are dangerous - use the do/while(0) idiom to protect against matching the wrong else:

#define FFLUSH(fp) do { if (fflush (fp) == EOF) perror ("fflush()"); } while (0)
#define FSYNC(fd) do {  if (fsync (fd) == -1) perror ("fsync()"); } while (0)

selectserver.c

There's a lot of unnecessary resetting of errno = 0: in most cases, we test function return values and only access errno if the function has indicated that it has written to it. We can trust that these functions behave as advertised, and eliminate those effectively-dead writes.

The signal handlers appear to be overkill, since we flush logs each time we write them anyway (and I think that responsibility ought to move to log.c, rather than mixed into the application code).

In init_addr() we only ever assign to status and never use it. So that variable can be eliminated.

In init_sock() we should close master_fd before we continue when setsockopt() fails. Actually, we shouldn't continue to the next iteration - just log the failure and attempt to bind regardless.

Similarly, when we fail to listen(), we also leak the file descriptor.

accept_new_connection() also leaks fds - whenever we return something other than slave_fd, we need to ensure that it is closed first. Again, consider proceeding despite failure to set options.

In read_line, we have if (!buf) { … free(buf); }. That's not wrong, but is redundant. And the subsequent status = 0 is pointless, as that's immediately followed by return (or perhaps *status = 0 was intended - it's not clear what the status values mean).

We could eliminate the duplication of the realloc() calls an consequent tests by starting with buf = NULL and moving the allocation to the beginning of the loop.

I think this is wrong:

               char *new = realloc (buf, page_size + page_size);

Perhaps total + page_size?

The redefinition of ARRAY_CARDINALITY() suffers the same problem as the first one.

Makefile

A lot of the warnings are enabled by -Wall -Wextra, so are redundant. I normally write them on separate lines, like this:

CFLAGS += -std=c17
CFLAGS += -O2
CFLAGS += -pedantic-errors
CFLAGS += -Wall
CFLAGS += -Wextra
⋮

That makes it easier to temporarily change the flags used by commenting out lines.

We don't need a %.o: %.c rule, as Make already knows about that, and if we just call the executable selectserver then Make can infer that too.

log.o shouldn't depend on selectserver.h - instead of writing dependencies yourself, just ask GCC to do it as a side-effect of compilation, like this:

CFLAGS += -MD
-include $(wildcard *.d)

We're missing .PHONY and .DELETE_ON_ERROR targets, which makefiles always need.

A replacement makefile is a bit simpler:

# the compiler:
CC = gcc-10

# compiler flags:
CFLAGS += -std=c17
CFLAGS += -Wall
CFLAGS += -Wextra
CFLAGS += -Warray-bounds
CFLAGS += -Wconversion
CFLAGS += -Wmissing-braces
CFLAGS += -Wno-parentheses
CFLAGS += -Wpedantic
CFLAGS += -Wstrict-prototypes
CFLAGS += -Wwrite-strings

CFLAGS += -fanalyzer
CFLAGS += -fno-builtin
CFLAGS += -fno-common
CFLAGS += -fno-omit-frame-pointer
CFLAGS += -fsanitize=address
CFLAGS += -fsanitize=undefined

CFLAGS += -O2


CFLAGS += -MD

selectserver: selectserver.o log.o

clean:
        $(RM) *.o *.d selectserver

-include $(wildcard *.d)

.PHONY: clean
.DELETE_ON_ERROR:

Answer 2

Answers to your questions

• Is my code POSIX compliant?

That question doesn't make sense. Your code does not implement the POSIX specification, so there is no question about compliance. You might use POSIX functions, but you are free to do that however you like.

• Do you see any outdated practices or functions?

Your macros, as Toby Speight already mentioned in his answer. Avoid them where possible: define static const variables and write static functions instead. If you really need a macro, make sure it is a safe one.

• Does any part of my code invoke undefined behaviour?

Make sure you check the return value of functions for errors. Even something like localtime() is allowed to fail and return NULL, in which case your code would crash because it tries to dereference a NULL pointer.

As Toby hinted at, the reallocation logic in read_line() is wrong. buf + retval can ultimately be larger than the size of the buffer, which is at most 2 * page_size.

It is possible to open more filedescriptors than an fd_set can handle.

• Is it portable (across UNIX-like systems)?

No. ioctl() and <sys/ioctl.h> are not part of POSIX for example. Also, not all UNIX-like systems are POSIX-compliant. On the other hand, your code looks very reasonable, and should work on most UNIX-like operating systems, but you might need to add some #ifdefs around some code to make it more portable.

• How much more information can I glean about a client from a given file descriptor?

That depends on the operating system. You can get the client's address using getpeername(), the local address used for the connection with the client using getsockname(), and more information might be possible using ioctl()s and getsockopt().

• Is there an alternative to errno/perror style of error checking?

I can recommend the BSD err() functions, they make it quite a lot easier, but they are not standard C or POSIX.

• Currently, I'm only logging the client's host and service name. What other sort of information should I log? Should I look into syslog?

That's up to you, what do you want to log? The syslog is one option, and syslog() is POSIX. On Linux, systemd will handle putting the stderr output into its logs for you. On other operating systems other logging mechanisms might be more preferrable.

• Do you see any potential buffer overflows?

Yes, as mentioned the receive buffer is not reallocated correctly.

Clean up resources on all error paths

If you call socket() but a subsequent setsockopt() fails, you forget to close the filedescriptor you got. This will eventually cause you to run out of filedescriptors. There are more cases where you forget to do this.

Remove unnecessary code.

If buf = realloc(0, ...) fails, you don't need to call free(buf). You also don't need to set errno = ENOMEM then, as the realloc() call will have done that for you.

You have a lot of error checking in your code, but sometimes it is fine to ignore certain errors. So what if setting SO_KEEPALIVE fails? The chat functionality will not be impacted by that.

It's not necessary to have exit_cleanup(); on process termination, filedescriptors will be closed automatically. If you are worried about parts of the log still being in the buffer and not being flushed, you should enable line buffering (using setlinebuf() for example). This will ensure the logs are not missing parts even if your program crashes without getting to the exit handler.

Keep main() simple

Your main() function is quite large. Make sure it (and other functions as well) is not too complex, so it will be easier to read and modify it. Consider splitting some of its functionality into other functions.