4
\$\begingroup\$

Code

#include <stdlib.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <string.h>
#include <signal.h>

void error(char *msg)
{
    perror(msg);
    exit(1);
}

char *get_file(char *request)
{

    int end, start, i;
    end = 4;
    start = 4;
    i = 4;

    while (*(request + i) != ' ')
    {
        end += 1;
        i += 1;
    }
    char *file = malloc(end - start + 1);
    memcpy(file, request + start, end - start);
    file[end - start] = '\0';

    return file;
}

void process_respond(char **reply, char *file)
{
    char *mine_type = strchr(file, '.');
    int lent = 18;
    *reply = (char *)malloc(18);
    bzero(*reply, 18);
    strcat(*reply, "HTTP/1.1 200 OK\n");

    if (!mine_type)
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: text/html\n\n"));
        strcat(*reply, "Content-Type: text/html\n\n");
    }
    else if (strcmp(mine_type, ".html") == 0)
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: text/html\n\n"));
        strcat(*reply, "Content-Type: text/html\n\n");
    }
    else if (strcmp(mine_type, ".js") == 0)
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: text/javascript\n\n"));
        strcat(*reply, "Content-Type: text/javascript\n\n");
    }
    else if (strcmp(mine_type, ".png") == 0)
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: image/png\n\n"));
        strcat(*reply, "Content-Type: image/png\n\n");
    }
    else if (strcmp(mine_type, ".gif") == 0)
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: image/gif\n\n"));
        strcat(*reply, "Content-Type: image/gif\n\n");
    }
    else if (strcmp(mine_type, ".svg") == 0)
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: image/svg+xml\n\n"));
        strcat(*reply, "Content-Type: image/svg+xml\n\n");
    }

    else if (strcmp(mine_type, ".css") == 0)
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: text/css\n\n"));
        strcat(*reply, "Content-Type: text/css\n\n");
    }

    else if (strcmp(mine_type, ".jpg") == 0)
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: image/jpeg\n\n"));
        strcat(*reply, "Content-Type: image/jpeg\n\n");
    }
    else
    {
        *reply = (char *)realloc(*reply, lent + strlen("Content-Type: text/html\n\n"));
        strcat(*reply, "Content-Type: text/html\n\n");
    }
}

int read_file_to_buffer(char *file, char **buffer, int *size_of_buffer)
{

    FILE *fptr;
    char public_dir[256] = "./public";
    if (strcmp(file, "/") == 0)
    {
        fptr = fopen("./public/index.html", "r");
    }
    else
    {
        strcat(public_dir, file);
        fptr = fopen(public_dir, "r");
    }

    if (!fptr)
    {
        return -1;
    }

    fseek(fptr, 0L, SEEK_END);
    *size_of_buffer = ftell(fptr);
    fseek(fptr, 0L, SEEK_SET);
    *buffer = (char *)malloc(*size_of_buffer);

    fread(*buffer, *size_of_buffer, 1, fptr);

    fclose(fptr);
    return 0;

    // What happened if we send a file that is too large?
}

void handle_sigint(int sig)
{
    exit(-1);
    printf("Server setting down\n");
}

int main(int argc, char *argv[])
{
    signal(SIGINT, handle_sigint);
    int sockfd, client_socketfd, client_len, portno;
    char buffer[BUFFER_LENGTH];
    struct sockaddr_in server_addr, client_addr; // server address

    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if (sockfd < 0)
    {
        error("Error when creating socket");
    }

    bzero(&server_addr, sizeof(server_addr));
    portno = htons(atoi(argv[1]));
    server_addr.sin_family = AF_INET;
    server_addr.sin_port = portno;
    server_addr.sin_addr.s_addr = INADDR_ANY;

    if (bind(sockfd, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0)
    {
        error("Can not bind the address to the socket");
    }

    listen(sockfd, 5);

    client_len = sizeof(client_addr);

    while (1)
    {
        int new_sockfd = accept(sockfd, (struct sockaddr *)&client_addr, &client_len);
        if (new_sockfd < 0)
        {
            error("Error on accept");
        }

        bzero(&buffer, BUFFER_LENGTH);
        if (read(new_sockfd, &buffer, BUFFER_LENGTH) < 0)
        {
            error("Can't not read message");
        }
        char *file = get_file(buffer);

        char *buffer_file;
        int size_of_buffer = 0;

        char *reply;

        int n = read_file_to_buffer(file, &buffer_file, &size_of_buffer);

        if (n == 0)
        {
            process_respond(&reply, file);

            if (write(new_sockfd, reply, strlen(reply)) < 0)
            {
                error("Can not write to the socket");
            }
            pid_t pid = fork();

            if (pid < 0)
            {
                exit(EXIT_FAILURE);
            }
            else if (pid == 0)
            {

                if (write(new_sockfd, buffer_file, size_of_buffer) < 0)
                {
                    error("Can not write to the socket");
                }
                exit(EXIT_SUCCESS);
            }
            else
            {
                wait(&pid);
            }
        }
        else
        {
            char *fail_message = "HTTP/1.1 400 Not Found\n"
                                 "Content-Type: text/html\n"
                                 "Content-Length: 27\n"
                                 "Connection: close\n"
                                 "\n"
                                 "Can not find the resources\n";

            if (write(new_sockfd, fail_message, strlen(fail_message)) < 0)
            {
                error("Can not write to the socket");
            }
        }

        if (buffer_file)
            free(buffer_file);
        if (file)
            free(file);
        if (reply)
            free(reply);

        if (close(new_sockfd) < 0)
        { // After close socket will have somtime before it unbind from a port
            error("Can not close listening socket");
        }
    }

    if (close(sockfd) < 0)
    {
        error("Can not close socket");
    }

    return 0;
}

Review request

I am currently learning how to write a simple HTTP server in C. My experience with C is still at beginner level, so I am looking for style, bad practices, and security issues.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ None of those realloc() calls are checked for failure before using the possibly-null pointer that's returned. You really need to fix that first. \$\endgroup\$ Feb 20 at 8:53
  • 2
    \$\begingroup\$ Also, it's full of security issues such as failing to check the length of the path, and lacking any checking for .. path compents (which should be done after decoding % escape sequences). \$\endgroup\$ Feb 20 at 8:56

1 Answer 1

3
\$\begingroup\$

The BSD standard provides some goodies in err.h. If the header is available on your system, you may be able to replace your custom error() function with BSD's err() (or some other version thereof).

That being said, the argument to error() should be of type const char * as it is not modified anywhere in the function.


bzero(*reply, 18);

From Linux's man page of bcopy():

Marked as LEGACY in POSIX.1-2001: use memcpy(3) or memmove(3) in new programs. Note that the first two arguments are interchanged for memcpy(3) and memmove(3). POSIX.1-2008 removes the specification of bcopy().

In other words, replace it with memcpy()/memmove().


The name get_file() appears rather ambiguous. I also do not understand what these magic values:

int end, start, i;
end = 4;
start = 4;
i = 4;

mean. Furthermore, the return value of malloc() goes unchecked in the function. In case of an error, malloc() returns a null pointer. Failure to check for it risks invoking undefined behavior by a subsequent null pointer dereference. The call to memcpy() following malloc() technically invokes undefined behavior too, because file might be NULL, and the C standard 7.1.4 states:

If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer, or a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) or a type (after promotion) not expected by a function with variable number of arguments, the behavior is undefined.

Here too the passed in char * is not modified, so this should be declared with the const qualifier too.


void handle_sigint(int sig)
{
    exit(-1);
    printf("Server setting down\n");
}

is plain undefined behavior.

According to §7.14.2.1:

If the signal occurs other than as the result of calling the abort or raise function, the behavior is undefined if the signal handler refers to any object with static or thread storage duration that is not a lock-free atomic object other than by assigning a value to an object declared as volatile sig_atomic_t, or the signal handler calls any function in the standard library other than — the abort function, — the _Exit function, — the quick_exit function, — the functions in <stdatomic.h> (except where explicitly stated otherwise) when the atomic arguments are lock-free, — the atomic_is_lock_free function with any atomic argument, or — the signal function with the first argument equal to the signal number corresponding to the signal that caused the invocation of the handler. Furthermore, if such a call to the signal function results in a SIG_ERR return, the value of errno is indeterminate.

These functions are async-signal-unsafe even in the POSIX standard. As alternatives, the POSIX standard does specify that _exit(), _Exit(), and write() are safe to be called in a signal handler.

But how do you report an error if write() fails and there's no other possible way to report an error? You don't.

But really, you shouldn't be using signal() in the first place. According to Linux's man page:

WARNING: the behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead.

I do not see why printing "Server setting down\n" on a SIGINT is necessary, or even useful. It is only serving to introduce more complexity in the code. And then you're exiting with an error status of -1 - which is another magic value - instead of EXIT_FAILURE from stdlib.h.

Ideally, the signal handler should only set a file-scope flag variable of type volatile sig_atomic_t which would be checked in main() before each iteration.

Though, you should disable SIGHUP because a closed connection would generate a send/recv error, which would kill the server process.

For which signal() suffices, as the man page says:

The only portable use of signal() is to set a signal's disposition to SIG_DFL or SIG_IGN.


Here's another problematic part of the code:

portno = htons(atoi(argv[1]));

Ignoring what the man page of atoi() says in the BUGS portion, we're accessing argv[1] without checking whether argc was greater than 1. Your program calls htons(atoi(argv[1])) with a null pointer when I run it without providing any arguments, so this too is undefined behavior.


You are exiting if bind() fails, and leaking the sockfd file descriptor.


The return value of listen() is ignored. Execution should not continue if listen() failed.


 char public_dir[256] = "./public";
 if (strcmp(file, "/") == 0)
 {
     fptr = fopen("./public/index.html", "r");

Consider defining macros for these strings so that they can be edited easily and in one place.


In read_file_to_buffer(), the return value of malloc() and fread() goes unchecked. Consider using true and false from stdbool.h instead of 0 and -1 for the return value.


In main(), fail_message should be of type const char *.

I doubt it matters much if we failed to close the server socket before exiting the program.

I would have expected to see send()/recv() in networking code rather than read()/write().

Calling exit() in the child branch of a fork() is a good way of
calling the functions registered with atexit(), flushing stdio buffers, deleting temporary files created with tmpfile() et cetera. Why use _exit rather than exit in the child branch of a fork?


In process_response(), the return value of strchr() goes unchecked. It may very well return a null pointer, dereferencing which would later invoke undefined behavior.

Then we have another magic value:

int lent = 18;
*reply = (char *)malloc(18);
bzero(*reply, 18);

This too holds no meaning to me. What is 18, and what does it denote?

The return value of malloc() goes unused here as well. There's also a needless cast. Starting with C89 (which is a 35 year old standard) malloc() and family returns a generic void * that is implicitly converted to and from any other pointer type. As such, its result need not be casted and only serves to clutter the code.

Then we have a lot of duplicate code. I'd refactor that into, perhaps, a table and pre-compute the lengths of the content-type headers.

realloc() is used incorrectly in the code. The return value should not be stored in the same pointer that is being reallocated. If realloc() fails, which you don't check for by ignoring the return value, it would return a null pointer which would overwrite the pointer to the original chunk of memory, and we'd have a memory leak in the code, and on subsequent requests, more memory leaks.


There's a BUFFER_LENGTH in main() whose definition I do not see in any part of the code.


*(request + i) is typically written as request[i].


main() is too long and does too much. Break it into smaller functions.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you. Your review is really detailed and concise. \$\endgroup\$ Feb 21 at 6:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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