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;

Yes. But remember the example is meant to show to use `snprintf()` rather than `malloc()`. — Roddy, Nov 12 '13 at 15:44
@Roddy you have a point but isn't an example supposed to be... exemplary ? — SirDarius, Nov 12 '13 at 15:47
it is not mandatory for the memory to be freed in the same function. The function can return p and p can be freed later in main for example. That's the whole point of malloc, manually control the availability of a memory portion — H_squared, Nov 12 '13 at 15:48
On second thought: in case the error n<0 occurs, p must be freed since it might cause memory leak. However, if the process stops/ends when NULL is returned, then it is no biggie, since the heap is automatically freed once the code is done executing. — H_squared, Nov 12 '13 at 15:54
@hhachem except the function presented here is not `main` but a function that could be executed many times in a long-running process. — SirDarius, Nov 12 '13 at 15:56
@SirDarius That is correct. But it can also run once, not fully execute and return NULL. If it is the only function to get executed, then the heap will be freed directly after. I do agree with you. I would never "not free" an allocated portion of the memory. I'm just pointing out the cases when it would not produce memory leak. — H_squared, Nov 12 '13 at 16:01
@SirDarius. 'exemplary' :) In an ideal world, maybe. But I'd prefer a minimally concise example to one that attempts to handle every possible error return from unrelated functions. That's 'an exercise for the reader'. — Roddy, Nov 12 '13 at 16:07
Good catch, now see https://www.kernel.org/doc/man-pages/reporting_bugs.html — , Nov 12 '13 at 18:35
@Wumpus Thanks. That was what I was looking for, as well as confirmation that I was really correct. — Roobie Nuby, Nov 12 '13 at 21:04
My `printf(3)` has a different example that simulates `asprintf`'s functionality with `snprintf`. It's dated 2011-09-28. — Fred Foo, Nov 21 '13 at 14:07

score 2 · Accepted Answer · answered Nov 21 '13 at 13:48

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.

score 0 · Answer 2 · answered Nov 19 '13 at 21:31

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.

snprintf man page example memory leak?

2 Answers2