3

So, for context, I have started writing a small HTTP-Server for learning purposes.

Currently, I have 2 modules: server and logger. The server module uses the logger I wrote internally for logging messages.

I will add the 2 modules here as follows: server.h + server.c, then Logger.h and Logger.c

struct server {
  int socket_fd;
  struct addrinfo *serv_info;
  char port[6];
  char ip_str[INET6_ADDRSTRLEN];
  int backlog;
};

server *server_init(server *serv, char *addr, char *port) {
  int status = 0;
  struct addrinfo hints = {0};

  if (serv == NULL)
    return NULL;

  if (port == NULL) {
    log_msg(COMMUNICATION, ERROR, "Please provide a non-null port to bind to");
    return NULL;
  }

  hints.ai_socktype = SOCK_STREAM;
  hints.ai_family = AF_UNSPEC;
  hints.ai_flags = AI_PASSIVE;

  status = getaddrinfo(addr, port, &hints, &serv->serv_info);
  if (status != 0) {
    log_msg(COMMUNICATION, ERROR, "GetAddrInfo error: [%d]", status); 
    return NULL;
  }

  struct addrinfo *p;
  for (p = serv->serv_info; p != NULL; p = p->ai_next) {
    serv->socket_fd = socket(p->ai_family, p->ai_socktype, p->ai_protocol);
    if (serv->socket_fd < 0) continue;

    int yes = 1;
    setsockopt(serv->socket_fd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(int));

    status = bind(serv->socket_fd, p->ai_addr, p->ai_addrlen);
    if (status < 0) {
      log_msg(COMMUNICATION, ERROR, "Could not bind to port: [%s]", port);
      goto err_cleanup;
    }

    /* finish up initializing the structure */
    serv->backlog = BACKLOG;
    memcpy(serv->port, port, strlen(port));
    serv->port[strlen(port)] = '\0';
    inet_pton(
        serv->serv_info->ai_family,
        serv->serv_info->ai_addr->sa_data,
        serv->ip_str
      ); 
    
    break;
  }

  if (p == NULL) {
    log_msg(COMMUNICATION, ERROR, "Failed to bind to any socket");
    goto err_cleanup;
  }
   
  status = listen(serv->socket_fd, serv->backlog);
  if (status < 0) {
    log_msg(COMMUNICATION, ERROR, "Failed to listen on socket");
    goto err_cleanup;
  }

  return serv;

err_cleanup:
  freeaddrinfo(serv->serv_info);
  return NULL;
}

And for the Logger:

#include <stdarg.h>

typedef enum {
  INFORMATION,
  DEBUG,
  WARNING, 
  ERROR,
  CRITICAL
} eLogLevel;

#define LOG_SUBS_COUNT 4

typedef enum {
  COMMUNICATION,
  DISPLAY,
  SYSTEM,
  SENSOR
} eLogSubSystem;

typedef struct s_log_struct {
  eLogLevel subs_levels[LOG_SUBS_COUNT];
  int fd; 
  int enabled;

  void (*output_func)(const char *msg); 
} Logger; 

void log_msg(eLogSubSystem system, eLogLevel level, const char *fmt, ...);

void log_msg(eLogSubSystem system, eLogLevel level, const char *fmt, ...) {
    pthread_mutex_lock(&lock);

    if (!gLogData || !gLogData->enabled || level < gLogData->subs_levels[system]) {
        pthread_mutex_unlock(&lock);
        return;
    }

    char buffer[1024]; 
    int offset = 0;

    rfc8601_timespec(buffer, 64); 
    offset = strlen(buffer);

    offset += snprintf(buffer + offset, sizeof(buffer) - offset, 
                       "[%s][%s] ", 
                       subsystem_to_str(system), 
                       level_to_str(level));

    if (offset < sizeof(buffer) - 1) {
        va_list args;
        va_start(args, fmt);
        offset += vsnprintf(buffer + offset, sizeof(buffer) - offset, fmt, args);
        va_end(args);
    }

    if (offset >= sizeof(buffer)) {
        offset = sizeof(buffer) - 2;
    }
    buffer[offset] = '\n';
    buffer[offset + 1] = '\0';

    gLogData->output_func(buffer);

    pthread_mutex_unlock(&lock);
}

I have included the only 2 relevant functions, I believe.

The thing is, on the line:

  if (port == NULL) {
    log_msg(COMMUNICATION, ERROR, "Please provide a non-null port to bind to");
    return NULL;
  }

When I send back a dummy GET response in my main:

    client_fd = accept(
        server.socket_fd,
        (struct sockaddr *)&client_addr,
        &client_size
      );
    if (client_fd < 0) {
      log_msg(COMMUNICATION, DEBUG, "Could not accept client");
      continue;
    }

    log_msg(COMMUNICATION, DEBUG, "Accepted client: [%d]", client_fd);
    response = "HTTP/1.0 200 OK \r\n"
               "\r\n"
               "Hello world\n";
    write(client_fd, response, 256);
    close(client_fd);

The output comes as:

HTTP/1.0 200 OK 

Hello world
Please provide a non-null port to bind tot\Llh8,Lh(|h(x0xd|^C 

Now, I know that I need to write the exact amount of characters I have in the response, that is not the question. The question is why does the log_msg get appended to the end of the response buffer? How do the 2 interact?

7
  • 4
    write(client_fd, response, 256); tells the computer to write 256 bytes starting from &response, which is not a "buffer" but a read-only string baked into the read-only part of your program along with all the other string-literals like "Please provide a non-null port...". Compilers will typically store all those string literals together, so where one ends, the next begins. Any \0 terminator chars will be ignored by write. Commented Nov 26 at 18:23
  • @Puscas Raul, What do you want to happen with memcpy(serv->port, port, strlen(port)); when strlen(port) would overflow char port[6]? Commented Nov 26 at 18:27
  • Ty for the answers. I will also change the memcpy so that it does not overflows. Commented Nov 26 at 18:49
  • 1
    @Barmar Oops, I missed the break - I think you're right (though I'm unsure about the continue at the top (it's been a long time since I last used BSD Sockets in C) Commented Nov 26 at 19:40
  • 1
    Maybe err_cleanup needs to close(serv->socket_fd). It depends on what the caller does with serv in this case. Commented Nov 26 at 19:43

2 Answers 2

2

You told the write function to read a buffer 256 bytes long. The buffer you sent it, i.e. the string pointed to by response, is not that long. That means you read past the bounds of an array, and doing so triggers undefined behavior in your code.

What likely happened in this particular case is that the string constant whose starting address was assigned to response happened to reside in memory right before the string constant you passed to a particular call to log_msg, so write continued to read past the end of one string constant into another.

But again, this is undefined behavior. There is no guarantee that this would happen in the general case. There's a good chance you'd see different behavior if you changed optimization settings or added extra calls to printf for debugging.

Sign up to request clarification or add additional context in comments.

Comments

0

The server outputs extra garbage bytes after hello world because you write 256 bytes... It does what you write, not what you want :)

To fix this problem, change the write call to

    write(client_fd, response, strlen(response));

Note however that write is a system call that is allowed to fail for various reasons or even perform a partial write. For more reliable operation, you should wrap this call in a loop that checks the return value and the value of errno to restart the call if possible.

ssize_t write_retry(int hd, const void *data, size_t len) {
    const unsigned char *p = data;
    ssize_t total_written = 0;
    while (len > 0) {
        ssize_t written = write(hd, p, len);
        if (written >= 0) {
            total_written += written;
            p += written;
            len -= written;
        } else {
            if (errno != EINTR)
                return -1;
        }
    }
    return total_written;
}

And use write_retry(client_fd, response, strlen(response)).

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.