Segmentation fault after realloc(). Can't assign allocated memory to pointer

Question

I'm trying to allocate some memory with realloc(). This works so far. But if I want to assign the allocated memory to a pointer in a struct variable, I get a segmentation fault:

// in header
typedef struct {
    int a;
    char test[20];
} MyContent;

typedef struct {
    MyContent* values; 
    // simmilar to: MyContent values[]
    // ... some other stuff
} MyData;

// in source
void myFunction(MyData* dataPtr) {
    dataPtr->values = NULL;
    MyData* tempPtr = NULL;

    for (int i = 1; i < 10; i++) {
        tempPtr = (MyContent*) realloc(dataPtr->values, i * sizeof(MyContent));
        if (tempPtr == NULL) {
            free(dataPtr->values);
            break;
        }
        dataPtr->values = tempPtr;  // Here I get the segmentation fault
        dataPtr->values[(i-1)].a = 42;
        // ...
    }
}

I can't figure out what's going wrong here. Any suggestions? Thanks for your help.

The error is in code you haven't pasted. The code above only has one significant issue -- it mishandles the case where `realloc` returns `NULL`. If you can post a complete, compilable example that shows the error, we can probably find it for you. Otherwise, run `valgrind` on your code. (By the way, is this C code or C++ code? You put both tags, and that makes it very confusing.) — David Schwartz, Apr 25 '12 at 07:11
@DavidSchwartz, the code the OP posted will compile and run under either C or C++ just fine :) — bdonlan, Apr 25 '12 at 07:13
@bdonlan: Right, so that makes it impossible to know what he's asking about. If I were going to test it, for example, should I test it as C code or C++ code? If I were going to suggest fixes/changes, should they be C or C++ code? — David Schwartz, Apr 25 '12 at 07:15
@DavidSchwartz, you could restrict yourself to the intersection of C and C++, which is basically most of C (with a few minor differences). It would be helpful to know for sure which the OP is using, but it's not impossible to try to help without knowing it. Just don't assume `void*` auto-casts, and don't name your variables `class` :) — bdonlan, Apr 25 '12 at 07:19
@user1355415, I think your last edit actually makes things worse - your casts are consistent with the sizeof now, but you're assigning to `dataPtr->values` which is `MyContent*` still... — bdonlan, Apr 25 '12 at 07:20
Thanks, sorry for the confusion. At first, I "corrected" the right one. Hope it fits now. It is (at least should be) C code, by the way. — fondor, Apr 25 '12 at 07:26
An off-topic question. Is it possible to view change history of a post? — tuxuday, Apr 25 '12 at 07:32
@tuxuday, click the timestamp after 'edited' above. eg, http://stackoverflow.com/posts/10310813/revisions — bdonlan, Apr 25 '12 at 07:38
@user1355415, you keep making subtle changes that might or might not affect the problems in your edited code. At this point I don't think we can really draw any conclusions about what your _real_ code might be doing. Try posting the real code, or just run valgrind on it and see what you can find yourself. — bdonlan, Apr 25 '12 at 07:39
thx @dbdonlan. The problem is that **tempPtr** is **MyData** whereas it should be **MyContent**. Once you do that change rest should fall in place automatically. — tuxuday, Apr 25 '12 at 07:52

score 1 · Accepted Answer · answered Apr 25 '12 at 07:28

Seems like you edited your code. The edited code works just fine.

#include<stdio.h>
#include<malloc.h>
#include<string.h>
// in header
typedef struct {
    int a;
    char test[20];
} MyContent;

typedef struct {
    MyContent* values; 
    // simmilar to: MyContent values[]
    // ... some other stuff
} MyData;

// in source
void myFunction(MyData* dataPtr) {
    dataPtr->values = NULL;
    MyData* tempPtr;

    for (int i = 1; i < 10; i++) {
        tempPtr = (MyData*) realloc(dataPtr->values, i * sizeof(MyContent));
        if (tempPtr == NULL) {
            if(dataPtr->values)
                free(dataPtr->values);
            printf("realloc() failed\n");
            return ;
        }
        dataPtr->values = (MyContent*)tempPtr;  // Here I get the segmentation fault
        dataPtr->values[(i-1)].a = 42+i;
        strcpy(dataPtr->values[(i-1)].test,"name");
    }
}

void PrintData(MyData* dataPtr) {
    for (int i = 1; i < 10; i++)
        printf("We have %s at %d\n",dataPtr->values[(i-1)].test,dataPtr->values[(i-1)].a);
}

main() {
    MyData Sample;
    myFunction(&Sample);
    PrintData(&Sample);
}

Sorry, for the changes in the code during all your answers. I tried to pull off everything that was unnecessary to figure out the problem. Doing so I might have left out or changed something in the wrong way. I realized that the argument I passed to `myFunction()` was errouneos what I now figured out due to your example post here. Thanks everyone - great and super fast support. — fondor, Apr 25 '12 at 08:19

score 0 · Answer 2 · edited Apr 25 '12 at 07:35

0

Your condition is wrong for checking the newly allocated memory. It should be:

if (tempPtr == NULL) {
  // handle error condition or continue with original 'dataPtr->values'
}
else {
  dataPtr->values = tempPtr;
}

Remember that realloc() doesn't necessarily transfer one block to the another block. Sometimes it may allocate the memory in the same pointer region.

edited Apr 25 '12 at 07:35

bdonlan

224,562
31
268
324

answered Apr 25 '12 at 07:08

iammilind

68,093
33
169
336

1

This is incorrect. You must not free the old pointer if it changed; realloc already took care of it for you. – bdonlan Apr 25 '12 at 07:10
The `reallocate` function will free the old block itself. You can't free it. – David Schwartz Apr 25 '12 at 07:10
Thanks, didn't know, that realloc() `free`s the old block itself. – fondor Apr 25 '12 at 08:26

bdonlan · Answer 3 · 2012-04-25T07:27:00.277

At first glance, I don't see a problem that could cause a crash there - that ones-based addressing is a bit odd, but not incorrect. There could be a problem in the code that you're not showing that results in heap or stack corruption that the realloc call makes worse. Or if you are compiling with optimizations, your debugger might be confused about where the crash is actually occurring. You're also confusing MyData and MyContent, but I'll assume that's just because you made an error while redacting the code.

Note also that if realloc fails, you will crash on the line after the one you indicated, as you'll be writing to a null pointer. You need to abort if tempPtr is NULL, not just free the old pointer. Again, though, this causes a fault on a different line than you indicated.

I'd recommend running your program under valgrind to see where it reports errors - the first such error to occur is likely to be the culprit.

Yes, I confused MyData and MyContent in the realloc() statement. Sorry for that, I corrected it. I also left out the `break`, which caused additional confusion. — fondor, Apr 25 '12 at 08:24

Segmentation fault after realloc(). Can't assign allocated memory to pointer

3 Answers3