Malloc errors when using POSIX threads on OSX

Question

I am currently completing final year assignments for my BSc in Computer Science one of which is on concurrency using POSIX threads. The assignment is a very simple http server that handles http requests in separate threads and returns simple html files. I have been experiencing some signal abort errors when freeing and nullifying the char buffers at the end of the thread functions. I am working in X-Code and when I enable Guard Malloc in the scheme it works just fine. Also if I remove the calls to free() and the nullification the program runs fine too. The latter I am assuming is consuming the memory used in the previous threads and not freeing it up for other processes and therefore is not a good solution, correct? I do not understand Guard Malloc but as I will hand in a raw .cpp file for compilation on the Lecturer's Linux machine the support will not be compiled.

Below is the complete code. Any comments, style critique or pearls of wisdom of any kind you are willing to throw my way are deeply appreciated.

The input that is crashing this is the .html homepage from this random website - http://www.budgie-info.com/

Many thanks in advance.

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <pthread.h>

// Define the port number to identify this process
#define MYPORT 3490

typedef struct socket_fd {
    unsigned fd;
} sock_fd;

void *request(void *fd);

int main() {
    int s;
    //unsigned fd;
    struct sockaddr_in my_addr;
    pthread_t t;
    int retval;

    sock_fd *s_fd;

    // Construct address information
    my_addr.sin_family = AF_INET;
    my_addr.sin_port = htons(MYPORT);
    my_addr.sin_addr.s_addr = INADDR_ANY;
    memset(my_addr.sin_zero, '\0', sizeof(my_addr.sin_zero) );

    // Create a socket and bind it the port MYPORT
    s=socket(PF_INET,SOCK_STREAM, 0);
    int yes = 1;
    setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(int));

    retval = bind(s, (struct sockaddr *)&my_addr, sizeof(my_addr));
    if( retval != 0) {
        fputs ("bind() failed.",stderr);
        exit (7);
    }

    printf("%d\n", my_addr.sin_port);
    retval = listen(s,10);
    if( retval != 0 ) {   // Allow up to 10 incoming connections
        fputs ("listen() failed.",stderr);
        exit (9);
    }

    while(1) {
        s_fd = (sock_fd*)malloc(sizeof(sock_fd));
        s_fd->fd=accept(s,NULL,NULL);             // wait for a request
        retval = pthread_create( &t, NULL, request, s_fd);
        free(s_fd);
        s_fd = NULL;
        if(retval != 0) {
            printf("pthread_create() failed. error %d\n", retval);
        }
    }
}

void *request(void *s_fd) {
    int retval;
    char data[65536];
    char header[]="HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n";
    char header_gif[]="HTTP/1.1 200 OK\r\nContent-Type: image/gif\r\n\r\n";
    char header_jpeg[]="HTTP/1.1 200 OK\r\nContent-Type: image/jpeg\r\n\r\n";
    char header_png[]="HTTP/1.1 200 OK\r\nContent-Type: image/png\r\n\r\n";
    int header_len;
    char *send_buffer;
    char *temp_buffer;
    char filename[256];
    char filename_parsed[256];
    FILE *f;

    sock_fd *fd_t = (sock_fd*)s_fd;
    unsigned fd = fd_t->fd;

    int cnt = 0;
    do {
        retval=(int)recv(fd,data,65536,0);              // recieve the request using fd
        ++cnt;
    } while(retval == 65536);

    if(retval < 0) {
        fputs ("recv() failed.\n",stderr);
        return NULL;
    }

    data[retval]='\0';                                  // NUL terminate it

    retval = sscanf((char *)data,"GET /%s ",filename);   // get the name of the file
    if(retval != 1) {
        fputs ("sscanf() failed.\n",stderr);
        return NULL;
    }
    if(strlen(filename) > 256) {
        fputs ("Filename overflow.\n",stderr);
        return NULL;
    }

    // parse uml spaces out of filenames
    int j = 0;
    for(int i = 0; filename[i]!='\0'; ++i) {
        if(filename[i] == '%') {
            filename_parsed[j] = ' ';
            i+=2;
        } else {
            filename_parsed[j] = filename[i];
        }
        ++j;
    }
    filename_parsed[j] = '\0';

    //print received header
    printf("Receiving:\n %s\n", data);
    //print requested filename
    printf("\n\n-------------filename = %s|\n\n", filename_parsed);

    if( (f=fopen(filename_parsed,"rb")) == NULL ) { // open the file (might be binary)
        fputs ("fopen() failed.\n",stderr);
        return NULL;
    }

    // obtain file size:
    size_t lSize;
    fseek (f , 0 , SEEK_END);
    lSize = ftell (f);
    rewind (f);

    // pre calculate length of filename
    int len = (int)strlen(filename_parsed);

    // identify appropriate header, alocate required memory and copy the header into the buffer
    if(0 == strcmp(filename_parsed + len - 4, ".gif")) {
        header_len = (int)strlen(header_gif);
        printf("\n\n\n\n\nG I F\n\n\n\n");
        send_buffer = (char*) malloc ( sizeof(char)*((lSize+(size_t)header_len)+14) ); // WARNING: hardcoded margin for header size differences
        memcpy(send_buffer, header_gif, header_len);
    } else if(0 == strcmp(filename_parsed + len - 5, ".jpeg")) {
        printf("\n\n\n\n\nJ P E G\n\n\n\n");
        header_len = (int)strlen(header_jpeg);
        send_buffer = (char*) malloc ( sizeof(char)*((lSize+(size_t)header_len)+14) ); // WARNING: hardcoded margin for header size differences
        memcpy(send_buffer, header_jpeg, header_len);
    } else if(0 == strcmp(filename_parsed + len - 4, ".png")) {
        header_len = (int)strlen(header_png);
        printf("\n\n\n\n\nP N G \n\n\n\n");
        send_buffer = (char*) malloc ( sizeof(char)*((lSize+(size_t)header_len)+14) ); // WARNING: hardcoded margin for header size differences
        memcpy(send_buffer, header_png, header_len);
    } else {
        header_len = (int)strlen(header);
        send_buffer = (char*) malloc ( sizeof(char)*((lSize+(size_t)header_len)+14) ); // WARNING: hardcoded margin for header size differences
        memcpy(send_buffer, header, header_len);
    }

    // allocate memory to contain the whole file:
    temp_buffer = (char*) malloc (sizeof(char)*(lSize+10));   // WARNING: hardcoded margin for header size differences
    if (temp_buffer == NULL) {
        fputs ("malloc() failed.\n",stderr);
        return NULL;
    }

    // copy the file into the buffer:
    retval = (int)fread (temp_buffer,1,lSize,f);
    if (retval != lSize) {
        fputs ("fread() failed.\n",stderr);
        return NULL;
    }

    memcpy(send_buffer + header_len, temp_buffer, retval);

    //print packet being sent
    printf("Sending:\n%s\n", send_buffer);

    memcpy(send_buffer + retval, "\r\n", 4);

    // send packet
    retval = (int)send(fd,send_buffer,retval,0);
    if(retval < 0) {
        fputs ("send() failed.\n",stderr);
        return NULL;
    }

    free(temp_buffer);    // The section of memory management causes SIGABRT errors everytime i run, commenting them out made it run smoothly.
    temp_buffer = NULL;   // Is the closing of this function tidying up for me or is it leaving a memory leak trail?
    free(send_buffer);    // Is it the multi threading that is making memory management buggy?
    send_buffer = NULL;
    fclose(f);
    f = NULL;

    close(fd);  // close the socket
    return NULL;
}

Answer 1

There are a bunch of problems with this code. It's hard to say why it's crashing on these files considering the number of potential problems (you did not provide a sample input that crashes your program)

• For this problem to be usable you need to set SO_REUSEADDR on the socket, otherwise if it is crashes (hint, hint), you will not be able to restart it right away and receive connections

• you do not check the return value of bind(), accept(), pthread_create()

• you need to call listen() only once, not in the loop

• it's safer to cast fd to long before to cast it to void * since that's what you do in listen. A cleaner way would involve a struct on the heap with an fd in it

• Do not define the size of arrays that you set at compile time, let the compiler do it. It's safer: char header[]="HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n";. Then you do not need to call strlen on it. You can just use sizeof(header) - 1.

• You do not check the return value of recv(). You must make sure it did not fail (esp since you use it as an index in an array)

• You do not ensure that filename is not overflowed. You read up to 65K, yet you do not make sure that filename is big enough (i.e it's trivial to crash your program and it's a security vulnerability)

• You do not read the return value of sscanf. There is no guarantee that filename has data

• You are not nul terminating filename_parsed (I believe it's the main reason your program is crashing)

• You might to call strlen(filename_parsed) only once and instead of calling it a zillion times

• Instead of your confusing character checks at the end, why not use strcmp (once filename_parsed is properly terminate of course): 0 == strcmp(filename_parsed + len - 5, ".jpeg") etc.

• Check the return value of send

Answer 2

You can check one thing: the content that's read from fread() will is not necessarifly a string - might not be NULL terminated at all. So using strcat on this is not the right way. Consider changing it to memcpy() or something.