Simple webserver in C

My first learning project in C. Looking for general feedback e.g. gotchas, coding standards, formatting, naming etc. I'm here to learn!

webserver.h

#ifndef __Webserver__webserver__
#define __Webserver__webserver__

#include 

struct webserver_connection {
    int status;
    int socket;
    char *path;
};

struct webserver_connection webserver_connect(int port_number, char *root);
void webserver_listen(struct webserver_connection);
void webserver_disconnect(struct webserver_connection);

#endif /* defined(__Webserver__webserver__) */

webserver.c

```
#include "webserver.h"
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include

struct webserver_request {
enum {REQUEST_POST, REQUEST_GET} method;
char *uri;
char *host;
};

struct webserver_response {
int status;
char *body;
unsigned long body_length;
char *content_type;
char *content_encoding;
};

int webserver_parse_lines(int client_socket, struct webserver_request *request);
int webserver_parse_line(char request_line[], struct webserver_request *request);
int webserver_accept_connection(struct webserver_connection server_connection);
int webserver_handle_connection(struct webserver_connection, int client_socket);
int webserver_respond(int client_socket, struct webserver_response *response);
void webserver_respond_with_error(int client_socket, int http_status);
int webserver_http_message(int http_status, char **http_message);
int webserver_get_content(char root_path, struct webserver_request request, char **file_content, unsigned long *file_length);
char webserver_content_type_from_filepath(char file_path);
void webserver_log_request(struct webserver_request *request, int http_status);

struct webserver_connection webserver_connect(int port_number, char *root) {

struct webserver_connection server_connection;
server_connection.status = 0;
server_connection.socket = socket(PF_INET, SOCK_STREAM, 0);

i

Are you sure that &(int) { 1 } does what you want it to do? I'm not sure it does.

Using malloc()/strcpy() is probably better handled with strdup() (which you've used elsewhere, so you're already okay with that).

In webserver_handle_connection, you're using malloc() to allocate space for an unsigned long. Why? Declare a normal local variable, and pass its address to webserver_get_content.

In webserver_parse_line, you have a free(token); statement in there. That statement doesn't do anything because token is already known to be NULL at the point it is reached. Also, there's no need to free anything when using strtok() because that function returns pointers into your buffer, not newly allocated memory. (This error occurs in a couple of places.)

In webserver_respond, you're sending an extra "\r\n" after the HTTP response body. This is a violation of the HTTP protocol and should not be sent.

In webserver_respond_with_error, you are using sprintf() with a (possibly) unknown string, printing into a fixed size buffer. What happens if the resulting string is 100 characters or more? You should first use snprintf() instead so you don't overwrite more than your buffer has allocated, and second you should work out how much space you actually need to allocate in the first place based on the message.

In webserver_get_content, often when a webserver gets a request for a directory name without a trailing slash, it will return an HTTP Redirect so that the client will resubmit the request with a trailing slash, so that relative links in the returned HTTP page go to the correct place.

In webserver_get_content, you're calling malloc(0) and then freeing that pointer later. That doesn't seem to make sense. Why? Also, loading the entire contents of a file into memory is reasonable for small files, but quite unreasonable for large files.

There's a data leak race condition when reading the file data - if the file is changed (say, truncated) between the time you get the size and the time you read the data, then you'll end up sending the (uninitialised) contents of memory space allocated by malloc(). This could have unintended security consequences.

In webserver_content_type_from_filepath, you've again got that pattern of malloc(0), then free(...), then malloc(...) again. Why?

In webserver_log_request, there is some code that is indicative of a pattern I've seen throughout your code. You've got a local buffer method (fine), which you strcpy() some data into and then use. There's no need to do this unnecessary copying. You can instead do:

const char *method = NULL; // initialise the pointer (good practice)

switch (request->method) {
    case REQUEST_GET:
        method = "GET";
        break;

Then, you can use method as usual. I've declared it as const char *, which is the type of a literal string (assuming you're using a reasonably modern C compiler). (I notice that you're not using any const at all. Your code would definitely benefit from that.)

In other places in your code (such as the handling of content_encoding), you do something similar where you strdup("utf-8") when you could be just assigning a pointer. Also, you fail to free() the content_encoding anywhere so you have a memory leak.

StackExchange Code Review Q#98255, answer score: 10