This is a follow up to my last question: Multiperson chat server using the select() API
Changes:
After much fine critique, I made the following changes to the code:
- Removed redundant casts and initializations.
- Moved the contents of selectserver.h to selectserver.c.
- Rewrote the
read_line()
function (Added checks for buffer overflow / DOS attack). - Enabled line-buffering for the file stream instead of flushing logs each time.
- Replaced the
FSYNC
macro with a function, and moved it to log.c. - Removed the
exit_cleanup()
function registered withatexit()
. - Simplified
main()
. Moved all the socket opening and file opening logic in separate functions. - Added error-handling for
time.h
functions. - Added a check for a file descriptor > 1024, silently closing it to guard against undefined behaviour.
Review goals:
Some more questions:
Could any of these syscalls block?
Is using blocking sockets defeating (at least partially) the point of
select()
?Is the overhead of continuously
malloc()
ing andfree()
ing comparable in contrast to the overhead of syscalls?Do you see a potential buffer overflow / memory leak? (Other than the
FILE *
valgrind reports about, though that should be eliminated as well)Does any part of my code exhibit undefined behaviour?
Is there a better strategy with handling file descriptors above 1024, other than gracefully exiting or silently closing any new connections? (Eliminating the possibility of using
poll()
and the like)Does any part of my code require a comment?
How do I avoid mixed messages? For instance:
Jack (typing): hel...
John (typing): nig...
Jack's screen: hellnight
- Would it suffice to call
fsync()
before closing the file descriptor once, instead of calling it after everywrite()
? - How can I further improve my code?
Code:
log.h:
#ifndef LOG_H
#define LOG_H
#include <stdio.h>
#include <unistd.h>
#include <time.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 /* 0b11111111 */
#define LOG_FULLTIME 0x03 /* 0b00000011 */
void flush_cache (int fd);
void log_msg (FILE * fp, const char *msg, unsigned int options);
#endif
log.c:
#include "log.h"
#define TS_BUF_LENGTH 30
void flush_cache (int fd)
{
if (fsync (fd) == -1) {
perror ("fsync()");
}
}
void log_msg (FILE * fp, const char *msg, unsigned int flags)
{
static long long log_count = 0;
char time_stamp[TS_BUF_LENGTH];
char date_stamp[TS_BUF_LENGTH];
time_t time_val = flags & LOG_FULLTIME ? time (0) : 0;
struct tm *tm_info = flags & LOG_FULLTIME ? localtime (&time_val) : 0;
if (flags & LOG_COUNT) {
fprintf (fp, "%lld\n, ", ++log_count);
}
if (flags & LOG_DATE
&& strftime (date_stamp, TS_BUF_LENGTH, "%F (%a)", tm_info)) {
fprintf (fp, "%s, ", date_stamp);
}
if (flags & LOG_TIME
&& strftime (time_stamp, TS_BUF_LENGTH, "%H:%M:%S", tm_info)) {
fprintf (fp, "%s, ", time_stamp);
}
fprintf (fp, "%s\n", msg);
}
selectserver.c
/*
* selectserver.c -- a multiperson chat server
*/
#define _POSIX_C_SOURCE 200809L
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
#include <errno.h>
#include <signal.h>
#include <unistd.h>
#include <sys/time.h>
#include <netdb.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <sys/ioctl.h>
#include <arpa/inet.h>
#include "log.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 LOG_FILE "server.log"
#define ARRAY_CARDINALITY(x) (sizeof(x) / sizeof (x)[0])
#define NI_MAXHOST 1024
#define NI_MAXSERV 35
FILE *log_fp = 0;
int log_fd = 0;
static void sigint_handler (int sig)
{
close (log_fd);
signal (sig, SIG_DFL);
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;
if (getaddrinfo (0, PORT, &hints, servinfo) != 0) {
perror ("getaddrinfo()");
return -1;
}
return 0;
}
static void configure_tcp (int master_fd)
{
int yes = 1;
socklen_t size_yes = sizeof yes;
if (setsockopt
(master_fd, SOL_SOCKET, SO_REUSEADDR, (void *) &yes,
size_yes) == -1) {
perror ("setsockopt()");
}
if (setsockopt
(master_fd, SOL_SOCKET, SO_KEEPALIVE, (void *) &yes,
size_yes) == -1) {
perror ("setsockopt()");
}
int keep_cnt = 9;
socklen_t size_cnt = sizeof keep_cnt;
if (setsockopt
(master_fd, IPPROTO_TCP, TCP_KEEPCNT, (void *) &keep_cnt,
size_cnt) == -1) {
perror ("setsockopt()");
}
int keep_idle = 25;
socklen_t size_idle = sizeof keep_idle;
if (setsockopt
(master_fd, IPPROTO_TCP, TCP_KEEPIDLE, (void *) &keep_idle,
size_idle) == -1) {
perror ("setsockopt()");
}
int keep_intvl = 25;
socklen_t size_intvl = sizeof keep_intvl;
if (setsockopt
(master_fd, IPPROTO_TCP, TCP_KEEPINTVL, (void *) &keep_intvl,
size_intvl) == -1) {
perror ("setsockopt()");
}
}
/* Returns: -1 on failure, a listening socket descriptor otherwise.
*/
static int open_tcp_socket (struct addrinfo **servinfo)
{
int master_fd = 0;
struct addrinfo *p = 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;
}
configure_tcp (master_fd);
if (bind (master_fd, p->ai_addr, p->ai_addrlen) == -1) {
close (master_fd);
perror ("bind()");
continue;
}
break;
}
if (!p) {
fprintf (stderr, "%s: Failed to setup a socket.\n", PROGRAM_NAME);
return -1;
}
if (listen (master_fd, SOMAXCONN) == -1) {
perror ("listen()");
close (master_fd);
return -1;
}
return master_fd;
}
static void get_slave_addr (int slave_fd, char *ip_str, int *port)
{
struct sockaddr_storage addr;
socklen_t addr_len = sizeof addr;
if ((getpeername (slave_fd, (struct sockaddr *) &addr, &addr_len)) ==
-1) {
perror ("getpeername");
}
/* Deal with both IPv4 and IPv6 */
if (addr.ss_family == AF_INET) {
struct sockaddr_in *s = (struct sockaddr_in *) &addr;
*port = ntohs (s->sin_port);
if (!inet_ntop (AF_INET, &s->sin_addr, ip_str, INET_ADDRSTRLEN)) {
perror ("inet_ntop()");
}
} else {
struct sockaddr_in6 *s = (struct sockaddr_in6 *) &addr;
*port = ntohs (s->sin6_port);
if (!inet_ntop (AF_INET6, &s->sin6_addr, ip_str, INET6_ADDRSTRLEN)) {
perror ("inet_ntop()");
}
}
}
static void write_slave_info (int slave_fd)
{
struct sockaddr_in slave_addr;
socklen_t addr_len = sizeof slave_addr;
char slave_ip[INET6_ADDRSTRLEN] = { 0 };
char local_ip[INET6_ADDRSTRLEN] = { 0 };
int port = 0;
get_slave_addr (slave_fd, slave_ip, &port);
if (getsockname (slave_fd, (struct sockaddr *) &slave_addr, &addr_len)
== -1) {
perror ("getsockname()");
}
if (!inet_ntop
(AF_INET, (const void *) &slave_addr, local_ip, sizeof local_ip)) {
perror ("inet_ntop()");
}
char host[NI_MAXHOST] = { 0 };
char service[NI_MAXSERV] = { 0 };
int ret_val = 0;
if ((ret_val =
getnameinfo ((struct sockaddr *) &slave_addr, addr_len, host,
sizeof host, service, sizeof service, 0)) != 0) {
fprintf (stderr, "%s: getnameinfo: %s\n", PROGRAM_NAME,
gai_strerror (ret_val));
}
char log_txt[MAX_LOG_TEXT] = { 0 };
snprintf (log_txt, sizeof log_txt,
"INFO: New connection from FOREIGN IP ADDRESS:%s, HOST:%s, SERVICE:%s,\n\t\t\t\t LOCAL IP ADDRESS:%s, PORT:%d on socket %d.",
slave_ip, host, service, local_ip, port, slave_fd);
log_msg (stderr, log_txt, LOG_FULLTIME);
log_msg (log_fp, log_txt, LOG_FULLTIME);
flush_cache (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;
do {
slave_fd = accept (master_fd, &slave_addr, &addr_len);
} while ((slave_fd == -1) && (errno == EINTR));
if (slave_fd == -1) {
perror ("accept()");
return -1;
}
write_slave_info (slave_fd);
return slave_fd;
}
static void write_farewell (int slave_fd)
{
char log_txt[MAX_LOG_TEXT] = { 0 };
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);
flush_cache (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;
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 *get_response (size_t *nbytes, int slave_fd, int *status)
{
static const size_t page_size = BUFSIZE;
char *buf = 0;
int flag = 0;
ssize_t ret_val = 0;
size_t total = 0;
do {
if (total > (BUFSIZE * 10)) { /* Likely a DOS attack */
free (buf);
*status = -1;
return 0;
}
char *new = realloc (buf, total + page_size);
if (!new) {
perror ("realloc()");
status = 0;
return 0;
}
buf = new;
new = 0;
ret_val = recv (slave_fd, buf + total, 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;
}
} else {
flag = 0;
}
} while (flag > 0);
if (ret_val <= 0) {
if (ret_val == 0) {
write_farewell (slave_fd);
}
free (buf);
*status = -1;
return 0;
}
*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) {
status = send (slave_fd, line + total, bytes_left, MSG_NOSIGNAL);
if ((status == -1) && (errno == EINTR)) { /* A send() error */
continue;
}
if (status == -1) {
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) {
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 (;;) {
/* 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) {
if (slave_fd < 1023) {
FD_SET (slave_fd, &master);
} else {
close (slave_fd);
continue;
}
}
if (slave_fd > fd_max) {
fd_max = slave_fd;
}
} /* We have data to read */
else {
size_t nbytes = 0;
int status = 0;
char *line = get_response (&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;
}
/* Returns: the master_fd on success, -1 otherwise.
*/
static int setup_server (void)
{
struct addrinfo *servinfo;
int master_fd = 0;
if (init_addr (&servinfo) == -1) {
return -1;
}
if ((master_fd = open_tcp_socket (&servinfo)) == -1) {
freeaddrinfo (servinfo);
return -1;
}
freeaddrinfo (servinfo);
return master_fd;
}
/* Returns: -1 on failure, 0 elsewise.
*/
static int open_logfile (void)
{
log_fp = fopen (LOG_FILE, "a");
if (!log_fp) {
perror ("fopen()");
return -1;
}
/* Be less efficient here, to solve a problem more important
* than mere efficiency
*/
if (setvbuf (log_fp, 0, _IOLBF, 0)) {
perror ("setvbuf()");
fclose (log_fp);
return -1;
}
log_fd = fileno (log_fp);
if (log_fd == -1) {
perror ("fileno()");
fclose (log_fp);
return -1;
}
return 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;
}
}
}
if (open_logfile () == -1) {
return EXIT_FAILURE;
}
int master_fd = 0;
if ((master_fd = setup_server ()) == -1) {
return EXIT_FAILURE;
}
/* Wait for and eventually handle a new connection */
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;
}
Dynamic analysis:
==387== Memcheck, a memory error detector
==387== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==387== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==387== Command: ./selectserver
==387== Parent PID: 325
==387==
==387== error calling PR_SET_PTRACER, vgdb might block
==387==
==387== Process terminating with default action of signal 2 (SIGINT)
==387== at 0x497FF7A: select (select.c:41)
==387== by 0x10A4FE: handle_connections (selectserver.c:372)
==387== by 0x10AB96: main (selectserver.c:504)
==387==
==387== HEAP SUMMARY:
==387== in use at exit: 472 bytes in 1 blocks
==387== total heap usage: 111 allocs, 110 frees, 56,793 bytes allocated
==387==
==387== 472 bytes in 1 blocks are still reachable in loss record 1 of 1
==387== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==387== by 0x48ED92D: __fopen_internal (iofopen.c:65)
==387== by 0x48ED92D: fopen@@GLIBC_2.2.5 (iofopen.c:86)
==387== by 0x10A884: open_logfile (selectserver.c:444)
==387== by 0x10AB40: main (selectserver.c:494)
==387==
==387== LEAK SUMMARY:
==387== definitely lost: 0 bytes in 0 blocks
==387== indirectly lost: 0 bytes in 0 blocks
==387== possibly lost: 0 bytes in 0 blocks
==387== still reachable: 472 bytes in 1 blocks
==387== suppressed: 0 bytes in 0 blocks
==387==
==387== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Edit: fixed typo.