Dynamic arrays: using realloc() without memory leaks

Question

I use realloc to resize the memory allocated:

char **get_channel_name(void)   
{
    char **result;
    int n;

    result = (char **) 0;
    for (elem = snd_mixer_first_elem(handle), n = 0; elem; elem = snd_mixer_elem_next(elem)) {
        if (!snd_mixer_selem_is_active(elem))
            continue;
        if (snd_mixer_selem_has_playback_volume(elem) &&
            snd_mixer_selem_has_playback_switch(elem) &&
            snd_mixer_selem_has_capture_switch(elem)) {
            if (result == (char **) 0)
                result = (char **) malloc(sizeof(char *));
            else
                result = (char **) realloc(result, sizeof(char *) * (n + 1)); /* nulled but not freed upon failure */
            result[n++] = strdup(snd_mixer_selem_get_name(elem));
        }
    }

    if (result == (char **) 0)
        return NULL;

    result = (char **) realloc(result, sizeof(char *) * (n + 1)); /* nulled but not freed upon failure */
    result[n] = NULL;

    return result;
}

When I check code with cppcheck tool static C/C++ code analysis, printed the following warings:

Common realloc mistake: 'result' nulled but not freed upon failure

How can I fix these 2 possible memory leaks?

Boz

Answer 1

If realloc() fails it returns NULL.

So if you do (and assuming realloc() would fail)

result = realloc(result, ...);

result will be assigned NULL and what it pointed to is not free()ed and the address to be free()ed is lost.

To fix this do:

{
  void * tmp = realloc(result, ...);
  if (NULL == tmp)
  {
    /* Handle error case, propably freeing what result is pointing to. */
  }
  else
  {
    result = tmp;
  }
}

Ti

Answer 2

The trick to fixing the "nulled but not freed upon failure" error is to store the value returned by realloc into a separate pointer, and check it for NULL before reassigning the old pointer:

char **tmp = (char **) realloc(result, sizeof(char *) * (n + 1));
if (tmp) {
    result = tmp;
} else {
    ... // Handle reallocation error
}

Now that the assignment of result is protected by NULL check, you have the old value to work with: you could free it if you want, or you could continue using it if you need to. The original code, on the other hand, does not give you the same option.

Note: When you pass NULL pointer to realloc, it behaves like malloc. That's why you can drop the conditional in the first use of realloc - replace this

if (result == (char **) 0)
    result = (char **) malloc(sizeof(char *));
else
    result = (char **) realloc(result, sizeof(char *) * (n + 1));

with this:

char** tmep = (char **) realloc(result, sizeof(char *) * (n + 1));
... // check temp and assign result here

Don't forget to set n to zero - currently, it's used uninitialized, which is undefined behavior.

Universe