C: memory error by using realloc

Question

Considering the toy-code as follows:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX_STRING_LENGTH (5000)

typedef struct request_body_s {
    char *data;
    size_t size;    // in bytes
} request_body_t;

int do_something(request_body_t *request_body) {
    char* content = read_content_elsewhere("/dir/content");
    size_t size = strlen(content) * sizeof(char);
    request_body->data = (char *) realloc(request_body->data, size + 1);
    if (request_body->data == NULL)
        return 0;
    else {
        request_body->size = size;
        strncpy(request_body->data, content, MAX_STRING_LENGTH);
        return 1;
    }
}

int main(int argc, char *argv[]) {
    request_body_t request_body;
    request_body.data = malloc(1);
    request_body.size = 0;

    if (do_something(&request_body))
        printf("Read!\n");
    else {
        printf("Error!\n");
        exit(0);
    }

    free(request_body.data);
    request_body.size = 0;
}

This code seems work fine until free(request_body.data) is called; it generates an error as follows:

*** free(): invalid next size (fast): 0x0000000001594570 ***

What is (of course) wrong and why? Thanks for any suggestion.

Your example program is missing certain parts, and also contains other errors. Please prepare a minimal, working example. — Thomas Padron-McCarthy, Sep 01 '15 at 15:29
You know that you may pass a null pointer to `realloc()`, in other words your first call to `malloc()` is not needed. — Cyclonecode, Sep 01 '15 at 15:31
`man strncpy(): [...]If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n bytes are written.[...]`. — EOF, Sep 01 '15 at 15:32
You should try using a debugger like valgrind in future. If you pass (the fixed version of) your code to valgrind it correctly notes that the error is on line 19 (`strncpy(...)`). Its a very useful tool for c development! — or1426, Sep 01 '15 at 15:36

score 4 · Answer 1 · answered Sep 01 '15 at 15:33

I believe the issue is right here:

 strncpy(request_body->data, content, MAX_STRING_LENGTH);

depending on your goal (not clear from your description), I would suggest:

strncpy(request_body->data, content, size > MAX_STRING_LENGTH ? MAX_STRING_LENGTH : size );

Jabberwocky · Answer 2 · 2015-09-01T15:44:53.660

2

strncpy copies the first n chars of the string, that is 5000 in your case. If the source string is smaller that n (5000 here), the rest is padded with zeros, therefore you are accessing further that the end of your bufffer, which leads to undefined behaviour.

You need:

strcpy(request_body->data, content);

It is safe here to use strcpy because we can be sure that the memory allocated by realloc is large enough, because you realloc strlen(content) + 1 chars.

BTW * sizeof(char) is always 1 by definition, so the * sizeof(char) is not necessary.

edited Sep 01 '15 at 15:44

answered Sep 01 '15 at 15:35

Jabberwocky

48,281
17
65
115

1

`request_body->data` has already been sized specifically for `content`, so using `strcpy` instead of `strncpy` is appropriate in this case. – dbush Sep 01 '15 at 15:37

score 0 · Answer 3 · edited May 23 '17 at 11:52

As written in the strncpy manual,

If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n bytes are written.

So, by using strncpy(request_body->data, content, 5000);, you write many '\0' outside your buffer. You shouldn't ever do that, it's an undefined behaviour, and, in this case, you're writing on the 'metadata' used by free, so it crashes.

Here, it would be preferable to use strcpy (and make sure to add a '\0' at the end), or memcpy, because you know the size you want to write.

Also, casting the return of malloc is useless, and sizeof(char) is and will very probably always be 1, so it's also useless.

C: memory error by using realloc

3 Answers3