snprintf man page example memory leak?

Question

The Linux man page for snprintf(3) give the following example:

#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>

char *
make_message(const char *fmt, ...)
{
    int n;
    int size = 100;     /* Guess we need no more than 100 bytes */
    char *p, *np;
    va_list ap;

    if ((p = malloc(size)) == NULL)
        return NULL;

    while (1) {

        /* Try to print in the allocated space */

        va_start(ap, fmt);
        n = vsnprintf(p, size, fmt, ap);
        va_end(ap);

        /* Check error code */

        if (n < 0)
            return NULL;

        /* If that worked, return the string */

        if (n < size)
            return p;

        /* Else try again with more space */

        size = n + 1;       /* Precisely what is needed */

        if ((np = realloc (p, size)) == NULL) {
            free(p);
            return NULL;
        } else {
            p = np;
        }
    }
}

After the /* check error code */ should this not be:

        if (n < 0) {
            free(p);
            return NULL;
        }

in order to avoid a memory leak?

I can't post this because the words to code ratio is not correct, so I have to add some more text at the end. Please ignore this paragraph, as the above is complete and to the point. I hope this is enough text to be acceptable.

BTW: I like the last line p = np;

Answer 1

Yes this code is leaky.

vsnprintf can return a negative number on error. In VC++, vsnprintf returns -1 when the target buffer is too small which break the logic in this code... See here: MSDN The VC implementation doesn't agree with the C standard...

Other sources for vsnprintf failure is sending a NULL 'format' buffer or bad encoding in the format buffer.

Answer 2

I don't know that strlen would ever return a value less than zero from within n = vsnprintf(...), but in the case that it did (and size > 0) this will definitely result in a memory leak.

The make_message function does a simple return NULL; without freeing the memory that it allocated with p = malloc(size). It is missing a free(p); just like you have stated in your original question.