manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
164 stars 50 forks source link

Reallocation re-use of invalid pointers #89

Closed lgarrison closed 7 years ago

lgarrison commented 7 years ago

In gridlink_impl.c.src, the reallocation code uses my_realloc to increase the amount of memory by MEMORY_INCREASE_FAC, but will try again with smaller values if any of the reallocations fail:

DOUBLE *posx=NULL, *posy=NULL, *posz=NULL;
do {
    posx = my_realloc(lattice[index].x,memsize,expected_n,"lattice.x");
    posy = my_realloc(lattice[index].y,memsize,expected_n,"lattice.y");
    posz = my_realloc(lattice[index].z,memsize,expected_n,"lattice.z");
    if(posx == NULL || posy == NULL || posz == NULL) {
        free(posx);free(posy);free(posz);
        expected_n--;
    }
} while(expected_n > nallocated[index] && (posx == NULL ||
                                           posy == NULL ||
                                           posz == NULL));

However, if the x realloc succeeds and y does not, I don't think it's valid to "try again" on the original x pointer. From the realloc docs:

On success, [...] The original pointer ptr is invalidated and any access to it is undefined behavior" http://en.cppreference.com/w/c/memory/realloc

So, if the x realloc succeeds, but y fails, the next iteration will try to realloc x, even though that pointer was invalidated by the first successful x realloc.

I think one solution would be to immediately assign successful reallocs to the original pointer and omit the free calls.

manodeep commented 7 years ago

Yup, this is a bug.

(Previously, my_realloc would crash if the realloc failed -- so my_realloc could never return a NULL pointer. )

manodeep commented 7 years ago

@lgarrison Does the fix look fine?

lgarrison commented 7 years ago

Looks good!