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
– Thomas Padron-McCarthy, Commented Sep 1, 2015 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
– Cyclonecode, Commented Sep 1, 2015 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.[...]. — user3185968
– user3185968, Commented Sep 1, 2015 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
– or1426, Commented Sep 1, 2015 at 15:36

malat · Accepted Answer · 2015-09-01 15:33:26Z

4

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

answered Sep 1, 2015 at 15:33

malat

12.7k15 gold badges103 silver badges200 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Jabberwocky · Accepted Answer · 2015-09-01 15:44:53Z

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 1, 2015 at 15:44

answered Sep 1, 2015 at 15:35

Jabberwocky

51.3k18 gold badges71 silver badges127 bronze badges

1 Comment

dbush Over a year ago

request_body->data has already been sized specifically for content, so using strcpy instead of strncpy is appropriate in this case.

Community · Accepted Answer · 2017-05-23 11:52:09Z

0

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.

edited May 23, 2017 at 11:52

CommunityBot

11 silver badge

answered Sep 1, 2015 at 16:57

4rzael

7096 silver badges19 bronze badges

Collectives™ on Stack Overflow

C: memory error by using realloc

3 Answers 3

Comments

1 Comment

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related