19
\$\begingroup\$

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 <stdio.h>

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 <sys/types.h>
#include <sys/socket.h>
#include <sys/select.h>
#include <sys/un.h>
#include <sys/stat.h>
#include <netinet/in.h>
#include <string.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <fcntl.h>

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);

    if (setsockopt(server_connection.socket, SOL_SOCKET, SO_REUSEADDR, &(int) { 1 }, sizeof(int)) < 0) {

        server_connection.status = -1;
        return server_connection;
    }

    server_connection.path = malloc(strlen(root) + 1);
    strcpy(server_connection.path, root);

    struct sockaddr_in socket_address;
    socket_address.sin_family = AF_INET;
    socket_address.sin_port = htons(port_number);
    socket_address.sin_addr.s_addr = htonl(INADDR_ANY);

    int length = sizeof(socket_address);
    if (bind(server_connection.socket, (struct sockaddr*) &socket_address, length) < 0) {

        server_connection.status = -1;
        return server_connection;
    }

    if (listen(server_connection.socket, 5) < 0) {
        server_connection.status = -1;
    }

    return server_connection;
};

void webserver_disconnect(struct webserver_connection server_connection) {
    close(server_connection.socket);
}

void webserver_listen(struct webserver_connection server_connection) {

    printf("Starts listening for new connections...\n");

    while (1) {

        struct timeval timeout;
        timeout.tv_sec = 1;
        timeout.tv_usec = 0;
        fd_set set;

        FD_ZERO(&set);
        FD_SET(server_connection.socket, &set);

        int status = select(server_connection.socket + 1, &set, NULL, NULL, &timeout);
        if (status > 0) {
            webserver_accept_connection(server_connection);
        }
    }
}

int webserver_accept_connection(struct webserver_connection server_connection) {

    int client_socket;
    struct sockaddr_in client_address;
    socklen_t client_length;

    client_socket = accept(server_connection.socket, (struct sockaddr*) &client_address, &client_length);
    if (client_socket < 0) {
        return -1;
    }

    pid_t pid = fork();
    if (pid == 0) {

        webserver_handle_connection(server_connection, client_socket);
        exit(0);
    } else {
        close(client_socket);
    }

    return 0;
}

int webserver_handle_connection(struct webserver_connection server_connection, int client_socket) {

    struct webserver_request request;
    int status;

    status = webserver_parse_lines(client_socket, &request);
    if (status == 0) {

        char *file_content;
        unsigned long *file_length = malloc(sizeof(unsigned long));

        status = webserver_get_content(server_connection.path, &request, &file_content, file_length);

        if (status == 0) {

            status = 200;
            char *content_type = webserver_content_type_from_filepath(request.uri);

            struct webserver_response response;
            response.status = status;
            response.body = file_content;
            response.body_length = *file_length;
            response.content_type = content_type;
            response.content_encoding = NULL;

            if (strcmp(content_type, "text/html") == 0) {
                response.content_encoding = strdup("utf-8");
            }

            webserver_respond(client_socket, &response);

            free(content_type);
        } else if (status > 0) {
            webserver_respond_with_error(client_socket, status);
        } else {
            webserver_respond_with_error(client_socket, 500);
        }

        free(file_content);
        free(file_length);
    }

    webserver_log_request(&request, status);

    close(client_socket);

    return 0;
}

int webserver_parse_lines(int client_socket, struct webserver_request *request) {

    char request_line[1024];

    while (1) {

        bzero(request_line, 1024);
        if (read(client_socket, request_line, 1023) < 0) {
            return -1;
        }

        int status = webserver_parse_line(request_line, request);

        if (status > 0) {
            webserver_respond_with_error(client_socket, status);
        } else {
            return status;
        }
    }

    return 0;
}

int webserver_parse_line(char request_line[], struct webserver_request *request) {

    char *token;
    int token_counter = 0;
    token = strtok(request_line, " ");

    while (token != NULL) {

        if (token_counter == 0) {

            if (strcmp(token, "GET") == 0) {
                request->method = REQUEST_GET;
            } else {
                return 400;
            }
        } else if (token_counter == 1) {

            request->uri = malloc(strlen(token));
            strcpy(request->uri, token);
        }

        token = strtok(NULL, " ");
        token_counter++;
    }

    free(token);

    return 0;
}

int webserver_http_message(int http_status, char **http_message) {

    *http_message = malloc(30);

    switch (http_status) {
        case 200:

            strcpy(*http_message, "OK");
            break;

        case 201:

            strcpy(*http_message, "Created");
            break;

        case 400:

            strcpy(*http_message, "Bad Request");
            break;

        case 401:

            strcpy(*http_message, "Unauthorized");
            break;

        case 403:

            strcpy(*http_message, "Forbidden");
            break;

        case 404:

            strcpy(*http_message, "Not Found");
            break;

        case 405:

            strcpy(*http_message, "Method Not Allowed");
            break;

        case 500:

            strcpy(*http_message, "Internal Server Error");
            break;

        default:

            return -1;
            break;
    }

    return 0;
}

int webserver_respond(int client_socket, struct webserver_response *response) {

    char *http_status_message;
    char response_headers[200];

    webserver_http_message(response->status, &http_status_message);
    size_t content_type_length = strlen(response->content_type + 11);
    if (response->content_encoding != NULL) {
        content_type_length += strlen(response->content_encoding);
    }

    char *content_type = malloc(content_type_length);
    strcpy(content_type, response->content_type);

    if (response->content_encoding != NULL) {

        strcat(content_type, "; charset=");
        strcat(content_type, response->content_encoding);
    }

    sprintf(
        response_headers,
        "HTTP/1.0 %d %s\nContent-Type: %s\nContent-Length: %lu\nServer: Webserver C\r\n\r\n",
        response->status,
        http_status_message,
        content_type,
        response->body_length
    );

    write(client_socket, response_headers, strlen(response_headers));
    write(client_socket, response->body, response->body_length);
    write(client_socket, "\r\n", strlen("\r\n"));

    free(http_status_message);
    free(content_type);

    return 0;
}

void webserver_respond_with_error(int client_socket, int http_status) {

    char *message;
    char *html = malloc(100);

    webserver_http_message(http_status, &message);
    sprintf(html, "<html><body>%s</body></html>", message);

    struct webserver_response response;
    response.status = http_status;
    response.body = html;
    response.body_length = strlen(html);
    response.content_type = strdup("text/html");
    response.content_encoding = strdup("utf-8");

    webserver_respond(client_socket, &response);

    free(message);
    free(html);
}

int webserver_get_content(char *root_path, struct webserver_request *request, char **file_content, unsigned long *file_length) {

    char *file_path = malloc(strlen(root_path) + strlen(request->uri) + 1);
    strcpy(file_path, root_path);
    strcat(file_path, request->uri);

    *file_content = malloc(0);

    if (access(file_path, F_OK) == -1) {
        return 404;
    }

    struct stat s;
    if (stat(file_path, &s) == 0) {
        if (s.st_mode & S_IFDIR) {
            return 404;
        }
    } else {
        return 404;
    }

    FILE *file;
    file = fopen(file_path, "rb");
    if (!file) {
        return -1;
    }

    fseek(file, 0, SEEK_END);
    *file_length = ftell(file);
    fseek(file, 0, SEEK_SET);

    free(*file_content);
    *file_content = malloc(*file_length + 1);
    fread(*file_content, 1, *file_length, file);

    fclose(file);

    return 0;
}

char* webserver_content_type_from_filepath(char *file_path) {

    char *file_path_copy = strdup(file_path);
    char *token;
    char *extension;
    char *content_type = malloc(20);

    extension = malloc(0);
    while ((token = strsep(&file_path_copy, ".")) != NULL) {

        free(extension);
        extension = strdup(token);
    }

    free(file_path_copy);
    free(token);

    if (strcmp(extension, "htm") == 0 || strcmp(extension, "html") == 0) {
        strcpy(content_type, "text/html");
    } else if (strcmp(extension, "jpg") == 0) {
        strcpy(content_type, "image/jpeg");
    } else if (strcmp(extension, "png") == 0) {
        strcpy(content_type, "image/png");
    } else if (strcmp(extension, "gif") == 0) {
        strcpy(content_type, "image/gif");
    }

    return content_type;
}

void webserver_log_request(struct webserver_request *request, int http_status) {

    char method[5];

    switch (request->method) {
        case REQUEST_GET:

            strcpy(method, "GET");
            break;

        case REQUEST_POST:

            strcpy(method, "POST");
            break;
    }

    printf("%s %s %d\n", method, request->uri, http_status);
}

main.c

#include "webserver.h"
#include <signal.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>

int main(int argc, const char * argv[]) {

    struct webserver_connection connection;
    connection = webserver_connect(8888, "/path/to/www-root");

    if (connection.status != 0) {

        perror("Webserver");
        return EXIT_FAILURE;
    }

    webserver_listen(connection);

    return EXIT_SUCCESS;
}
\$\endgroup\$
  • \$\begingroup\$ leading underscores are a very bad idea as the compiler places a leading underscore (or two) on every symbol it defines/extracts into its' symbol table. \$\endgroup\$ – user3629249 Jul 27 '15 at 20:45
  • \$\begingroup\$ the file webserver.h contains nothing that needs any part of stdio.h, so that line should be removed \$\endgroup\$ – user3629249 Jul 27 '15 at 20:48
  • \$\begingroup\$ the declaration of the main function has two parameters that are not used. This causes the compiler to raise two warning about unused parameters. Suggest: 'int main( void )' Note: when compiling always enable all warnings, then fix those warnings. for gcc, enable 'most' warnings by 'gcc -Wall -Wextra -pedantic' \$\endgroup\$ – user3629249 Jul 27 '15 at 20:51
  • \$\begingroup\$ when passing a enum parameter. Best to define the enum in the header file with a typename then use that typename in the parameter list of the function. that way, the enum is only defined once and can be used everywhere the header file is included \$\endgroup\$ – user3629249 Jul 27 '15 at 20:54
  • 2
    \$\begingroup\$ @user3629249: Might be better to move your comments to an actual answer, that's what it's for :) \$\endgroup\$ – Greg Hewgill Jul 27 '15 at 21:03
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your input! The malloc(0) and free() was sort of a way to get around the problem with freeing a pointer that wasn't allocated. By initializing the pointer with NULL this problem disappears and I can remove it. \$\endgroup\$ – brasimon Jul 27 '15 at 21:19
  • \$\begingroup\$ If I doesn't use malloc or strdup when using char*. Don't I have to free the pointer? \$\endgroup\$ – brasimon Jul 27 '15 at 21:57
  • 1
    \$\begingroup\$ @brasimon: You only free() memory that has been allocated with malloc() (and strdup() calls malloc()). A statement like method = "GET" makes the pointer method point to the constant string "GET" which the compiler helpfully provides for you embedded in your executable. You can't modify what the string the pointer points to, and you don't free it because nothing was allocated. The string already exists and will continue to exist until your program exits. \$\endgroup\$ – Greg Hewgill Jul 27 '15 at 22:21
2
\$\begingroup\$

There's no documentation whatsoever in your code. I know being a web server is self-explanatory but might not be for everyone. Also, it is recommended that all 'struct' definitions to go into the header file. The header file should have enough information so a that you don't even need to look into its '.c' counterpart/header-implementation.

\$\endgroup\$
  • 1
    \$\begingroup\$ that's a somewhat subjective matter - code with good variable and function names can be self documenting. \$\endgroup\$ – Alnitak Jan 5 '16 at 8:01
  • \$\begingroup\$ You are right. I don't have any arguments against that. His function names are self explanatory. \$\endgroup\$ – jarr Jan 7 '16 at 14:32
0
\$\begingroup\$

In addition to the malloc() etc related stuff in Greg's answer, there's actually no need whatsoever to use select() when you're only handling a single (blocking) file descriptor.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.