0

I'm having trouble with allocating memory correctly.

p is a pointer that I intended to point to method, target, then http switching to the next one after each space is found.

Then I thought I'd be able to test each char* value, but I receive an error in valgrind. Can anyone help me figure out what is going on here?

GET / HTTP/1.1
==9554== Invalid read of size 1
==9554==    at 0x4C2E8C9: strncmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9554==    by 0x4026C3: parse (server.c:713)
==9554==    by 0x4018CA: main (server.c:186)
==9554==  Address 0x55031a0 is 0 bytes inside a block of size 1 free'd
==9554==    at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9554==    by 0x4025D6: parse (server.c:699)
==9554==    by 0x4018CA: main (server.c:186)
==9554== 

Parse Function

bool parse(const char* line, char* abs_path, char* query)
{
    char* method;
    char* target;
    char* http;
    char* p;
    int i = 0;
    int size = 0;
    int place = 0;
    method = malloc(sizeof(char));
    target = malloc(sizeof(char));
    http = malloc(sizeof(char));
    p = method;
    do {
        if(line[i] == ' ') {
            size = 0;
            place++;
            i++;
            if(place == 1)
                p = target;
            if(place == 2)
                p = http;
        }
        // I want to increase the size of memory available to *p by 1 char
        p = realloc(p, sizeof(char)*(size+1));
        p[size] = line[i];
        i++;
        size++;
        if(line[i] == ' ' || line[i] == '\r') {
            p = realloc(p, sizeof(char)*(size+1));
            p[size] = '\0';
        }
        if(p == NULL) {
            error(501);
            return false;
        }
    } while (line[i] != '\r');

    // This is where I'm receiving an invalid read.
    if(strncmp(method, "GET\0", 4) != 0) {
        error(405);
        return false;
    }

    if(strncmp(target, "/", 1) != 0) {
        error(501);
        return false;
    }

    if(strstr(target, "\"")) {
        error(400);
        return false;
    }

    if(strncmp(http, "HTTP/1.1\0", 9) != 0) {
        error(505);
        return false;
    }

    strcpy(abs_path, target);
    query = NULL;

    return true;
}

Oddly this still seems to pass the tests:

:) server.c exists
:) server compiles
:) HTTP/1.0 returns error code 505
:) Method of 'abcGET' returns error code 405
:) Method of 'GETabc' returns error code 405
:) request-target without starting '/' returns error code 501
:) request-target of abc/hello.php returns error code 501
:) Requesting cat.exe returns error code 501
:) Requesting non-existant file returns error code 404
:) Requesting request-target with " returns error code 400
:) Two spaces after GET returns error code
:) A space within the request target returns error code
:) Two spaces before HTTP/1.1 returns error code
1

This is a tough one. It has to do with reusing p and realloc. I cannot find a specific technical explanation, but regardless, there are things that are problematic with this approach.

First, you cannot depend on realloc to keep the same address. It'll start out with p = method;, ie p pointing to the same address as method. But if realloc "moves" p, then method (and/or target and/or http) are goners. Once load and indexes are implemented, it may in fact provoke such a result.

Second, valgrind should also be reporting a leak, since there are mallocs without accompanying frees. I did a repro, and couldn't get rid of the leaks. I added the 3 frees, (method, target, http). No dice.

There's a couple of ways you could change this and preserve the structure and the code. For instance, allocate method, target, http (since each is a substring of line, none will be bigger than the length of line). Then remove the reallocs. Or, make p an array; it could be subscripted by place.

Memory leaks lead to "unpredictable results"; check50/server1 is fairly generous on that score. check50/server2 is more sensitive to leaks because more code is in play (queries, images, load, indexes).

6
  • I wondered if it might be a bit weird to do that, but then it seemed to make some sense and passed the tests ok. I've ignored this for the time being, and check50/server2 is passing tests too (well except for queries and case which I've yet to implement). – OrderAndChaos Nov 22 '16 at 1:25
  • So if p is pointing to method and realloc changes the address, method will be pointing to the old address still? – OrderAndChaos Nov 22 '16 at 1:27
  • I'll look at how to use strtok I think I can use it to split the string on the spaces. – OrderAndChaos Nov 22 '16 at 1:30
  • The issue seems to have been the realloc, I have simply malloced method, target and http to be fixed lengths (so I actually don't need to malloc, I could do something like method[strlen(line)]) and used a single return at the end of the function so I can free all of them before the return. Thanks for the help :) – OrderAndChaos Nov 22 '16 at 2:00
  • Glad to hear, glad to help. And re: comment 2, if realloc changes p's address, method will not point to that address. I assume it's still pointing to the old address; the testing/repro I did gave "unpredictable results" :) – DinoCoderSaurus Nov 22 '16 at 2:52

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .