views:

363

answers:

4

What is the best way to zero out new memory after calling realloc while keeping the initially allocated memory intact?

#include <stdlib.h>
#include <assert.h>
#include <string.h>
#include <stdio.h>

size_t COLORCOUNT = 4;

typedef struct rgb_t {
    int r;
    int g;
    int b;
} rgb_t;

rgb_t** colors;

void addColor(size_t i, int r, int g, int b) {
    rgb_t* color;
    if (i >= COLORCOUNT) {
        // new memory wont be NULL
        colors = realloc(colors, sizeof(rgb_t*) * i);
       //something messy like this...
        //memset(colors[COLORCOUNT-1],0 ,sizeof(rgb_t*) * (i - COLORCOUNT - 1));

         // ...or just do this (EDIT)
        for (j=COLORCOUNT; j<i; j++) {
            colors[j] = NULL;
        }

        COLORCOUNT = i;
    }

    color = malloc(sizeof(rgb_t));
    color->r = r;
    color->g = g;
    color->b = b;

    colors[i] = color;
}

void freeColors() {
    size_t i;
    for (i=0; i<COLORCOUNT; i++) {
        printf("%x\n", colors[i]);
        // can't do this if memory isn't NULL
       // if (colors[i])
         //   free(colors[i]);

    }
}


int main() {
    colors = malloc(sizeof(rgb_t*) * COLORCOUNT);
    memset(colors,0,sizeof(rgb_t*) * COLORCOUNT);
    addColor(0, 255, 0, 0);
    addColor(3, 255, 255, 0);
    addColor(7, 0, 255, 0);


    freeColors();
    getchar();
}
+2  A: 

First of all, realloc can fail, so you need to check for NULL. Second, there is no better way to zero out the memory: just memset from the end of the old buffer to the end of the larger buffer.

florin
my commented out section crashes visual studio with "msvcr90d.dll!memset(unsigned char * dst=0x00000000, unsigned char value='', unsigned long count=2553828) Line 103 Asm". Am I just off by 1?
Nick
it looks like you're passing the value at colors[COLORCOUNT-1] to be reallocated, rather than a pointer to that location.
atk
realloc may not be able to reallocate in-place and will return a new pointer. You should check this and update color with the new value if it is not NULL. That's one reason the code may fail. The other reason may be that realloc couldn't allocate more memory (it returned NULL to tell you this, but you discard it).
Draemon
All bytes zero may or may not be a null-pointer constant. See my answer for details.
Alok
+1  A: 

Write your own function, say reallocz, that accepts the current size as a parameter, and calls realloc and memset for you. It really won't get much better than what you already have... it's C, after all.

Thomas
+2  A: 

There is no way to solve this as a general pattern. The reason why is that in order to know what part of the buffer is new you need to know how long the old buffer was. It's not possible to determine this in C and hence prevents a general solution.

However you could write a wrapper like so

void* realloc_zero(void* pBuffer, size_t oldSize, size_t newSize) {
  void* pNew = realloc(pBuffer, newSize);
  if ( newSize > oldSize && pNew ) {
    size_t diff = newSize - oldSize;
    void* pStart = ((char*)pNew) + oldSize;
    memset(pStart, 0, diff);
  }
  return pNew;
}
JaredPar
+2  A: 

There is probably no need to do the memset: you may not be using colors[k] before setting it with something valid later. For example, your code sets colors[i] to a newly allocated color pointer, so you didn't need to set colors[i] to NULL.

But, even if you wanted to "zero it out so everything is nice", or really need the new pointers to be NULL: the C standard doesn't guarantee that all-bits-zero is the null pointer constant (i.e., NULL), so memset() isn't the right solution anyway.

The only portable thing you can do is to set each pointer to NULL in a loop:

size_t k;
for (k=COLORCOUNT; k < i+1; ++k) /* see below for why i+1 */
    colors[k] = NULL;

Your primary problem is that your realloc() call is wrong. realloc() returns a pointer to the resized memory, it doesn't (necessarily) resize it in-place.

So, you should do:

/* i+1 because you later assign to colors[i] */
rgb_t **tmp = realloc(colors, (i+1) * sizeof *tmp);
if (tmp != NULL) {
    /* realloc succeeded, can't use colors anymore */
    colors = tmp;
} else {
    /* realloc failed, colors is still valid */
}

If you really want to know what the memset() call should be, you need to set to zero the memory starting at colors+COLORCOUNT, and set i+1-COLORCOUNT members to zero:

memset(colors+COLORCOUNT, 0, (i+1-COLORCOUNT) * sizeof *colors);

But as I said above, all bytes zero is not guaranteed to be a NULL pointer, so your memset() is useless anyway. You have to use a loop if you want NULL pointers.

Alok
Thanks - the realloc error was an oversight while quickly writing up the test but I did not realize all bytes zero does not guarantee to NULL all of array members as per standard.
Nick