CODEWITHSUNDEEP
cmemory-managementheap-memory
vdenotaris
vdenotaris

Reputation: 13637

C: memory error by using realloc

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.

Upvotes: 0

Views: 174

Answers (3)

4rzael
4rzael

Reputation: 709

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.

Upvotes: 0

Jabberwocky
Jabberwocky

Reputation: 50776

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.

Upvotes: 2

malat
malat

Reputation: 12489

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 );

Upvotes: 4

Related Questions