Pset6 Server seg fault in load and main (because of parse)

Question

Server is functional and gives the correct output using the curl requests individually. However, using gdb, if I use curl to request hello.html after I've requested cat.html (specifically), I get a seg fault on this line(in load):

buffer = malloc(sizeof(BYTE) * size);

and using gdb, if I use curl to request cat.html and then request cat.html again (specifically), I get a seg fault on this line(in main):

path = malloc(strlen(root) + strlen(p) + 1);

which is what follows the parse function, so I assume it's seg faulting because of how I implemented parse.

Can someone please help me out?

Here's my load:

#define CHUNK 1024

bool load(FILE* file, BYTE** content, size_t* length)
{
BYTE *buffer, *buffer2;
size_t size, temp;

size = CHUNK;
temp = size;

buffer = malloc(sizeof(BYTE) * size);

size_t cur_len = 0;

while ((cur_len = fread(buffer, sizeof(BYTE), CHUNK, file)) > 0) 
{
    size += cur_len;

    if (temp - size < CHUNK) 
    {
        // buffer is too small, the next read could overflow!
        temp *= 2;                      
        buffer2 = malloc(sizeof(BYTE) * temp);
        memcpy(buffer2, buffer, size * sizeof(BYTE));
        free(buffer);
        buffer = buffer2;
    }
}
strcat(buffer, "\0");
*content = buffer;
*length = size;

return true;
}

and here's my parse:

bool parse(const char* line, char* abs_path, char* query)
{
// Extract 'method' string from 'line' string
int line_len = strlen(line);
char* temp = malloc(line_len);
char* method = malloc(line_len);
strcpy(temp, line);
strcpy(method, temp);
method = strtok(temp, " ");

// method spec
if((strlen(method)) != 3)
{
    error(405);
    return false;
}

if((strcasecmp(method, "get")) != 0)
{
    error(405);
    return false;
}

int j = 0;
while(j < line_len)
{
    // only one space separating strings
    if((line[j] == ' ') && (line[j + 1] == ' '))
    {
        error(400);
        return false;
    }

    // filename must not have " 
    if (line[j] == '"')
    {
        error(400);
        return false;
    }

    j++;
}

// HTTP Version spec
char* version = strcasestr(line, "HTTP/1.1");
if(version == NULL)
{
    error(505);
    return false;
}

// .exe is not valid
char* exe_file = strcasestr(line, ".exe");
if(exe_file != NULL)
{
    error(501);
    return false;
}

// Extract 'request-target' string from 'line' string
char* temp1 = malloc(line_len);
char* req_target = malloc(line_len);
strcpy(temp1, line);
strcpy(req_target, temp1);

req_target = strtok(temp1, " ");

if(strstr(line, "?") != NULL)
{
    req_target = strtok(NULL, "?");
}
else
{
    req_target = strtok(NULL, " ");
}

int tar_len = (strlen(req_target)) + 1;
char* temp2 = malloc(tar_len);
strcpy(temp2, req_target);
strcat(temp2, "\0");

// req_target spec
if(temp2[0] != '/')
{
    error(501);
    return false;
}

// CRCF spec
char* CRCF = strstr(line, "\r\n");
if(CRCF == NULL)
{
    error(400);
    return false;
}

 // set new abs_path
strcpy(abs_path, temp2);

// Extract 'query' string from 'line' string
char* temp3 = malloc(line_len);
char* the_query = malloc(line_len);
strcpy(temp3, line);
strcpy(the_query, temp3);

char* quest_m = strchr(the_query, '?');
if(quest_m != NULL)
{
    the_query = strtok(temp3, " ");
    the_query = strtok(NULL, "?");
    the_query = strtok(NULL, "  ");
}

int qry_len = strlen(the_query) + 2;

// check if query exists
if(the_query != NULL)
{
    char* char_eq = strchr(the_query, '=');

    if(char_eq != NULL)
    {
        char* qry = malloc(qry_len);
        strcpy(qry, the_query);
        strcat(qry, "\0");
        strcpy(query, qry);
    }
    else
    {
        char* qry = malloc(1);
        strcpy(qry, "\0");
    }
}
else
{
    char* qry = malloc(1);
    strcpy(qry, "\0");
}

return true;
}

Answer 1

Let's get this out of the way first. Don't do this strcat(buffer, "\0");. You need to return the exact contents of file to main. This changes the contents; it adds something that's not there. A browser will not complain about this little addition. valgrind and/or check50/server2 probably will. Assuming buffer is allocated for the exact length of the file, you've just added '\0' to memory that doesn't belong to buffer. Speaking of which.....

Now let's take /cat.jpg for a spin in load.

size = CHUNK; size=1024
temp = size; temp=1024
buffer = malloc(sizeof(BYTE) * size); buffer is [1024] (i.e. allocated for 1024 bytes)
(cur_len = fread(buffer, sizeof(BYTE), CHUNK, file)) first read, cur_len =1024
size += cur_len; size = 2048
if (temp - size < CHUNK) 1024 - 2048 < 1024, TRUE
temp *= 2; temp = 2048
buffer2 = malloc(sizeof(BYTE) * temp); buffer2 is [2048]
memcpy(buffer2, buffer, size * sizeof(BYTE)); copy 2048 bytes of buffer into buffer2.

WAT???? buffer only owns 1024 bytes. Who owns the other 1024?? (maybe temp that was allocated in parse?). Any file bigger than CHUNK bytes will provoke this, including favicon.ico at around 1406. If you are inclined to debug with gdb, that might be an easier file to work with than cat.jpg.

[WARNING: It is very important that you check my math. gdb does better arithmetic than I do (everybody does better arithmetic than I do!)]

This free(buffer); provokes the Address 0x550fb20 is 0 bytes after a block of size 1,024 alloc'd. message from valgrind. It likely explains the "double free" messages that pop up when you (correctly) add the frees to parse.

I have no "change this to that and all will be well". You kinda have to rethink your function. The CHUNK, cur_len approach is great. Your code indicates you know what needs to be done. The devil is in the details. The function will need realloc. And while you are rethinking, consider this: memcpy(buffer2, buffer, size * sizeof(BYTE)); is always copying the contents of buffer into the contents of buffer2, starting at the first byte.