Re: manpages-dev: printf(3) example: possible integer overflow

[walter harms <wharms-fPG8STNUNVg@public.gmane.org>]

Am 17.02.2016 13:40, schrieb Stéphane Aulery:
> retitle 794947 printf(3): possible integer overflow in make_message example
> severity 794947 wishlist
> tags 794947 + confirmed
> tags 794947 + upstream
> forwarded 794947 linux-man-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> stop
> 
> -----
> 
> Hello Walter,
> 
> Jakub Wilk reported a possible integer overflow in make_message example :
> 
>> The example in the printf(3) manpages looks like this (with boring parts
>> omitted):
>>
>> int n;
>> /* ... */
>>   n = vsnprintf(p, size, fmt, ap);
>>    /* ... */
>>    if (n < 0) {
>>        /* ... */
>>        return NULL;
>>    }
>>    /* ... */
>>    size = n + 1;
>>
>>
>> But vsnprintf could return INT_MAX, which would then cause "n + 1" to
>> overflow.
>>
>> (AFAICS, the glibc vsnprintf implementation never returns INT_MAX, but
>> it could in principle.)
>>
>> I'd suggest changing "n < 0" to "n < 0 || n == INT_MAX".
> 
> 

Hi,

the bug is real, the type of size should be size_t (in my original post it was int)
That would make the error check useless, so we would need to store
the vsnprintf return value in an int.

The problem is that the idea was to have a simple example and cluttering
it with error checks will make it hard to read. How many people would
notice that size_t is unsigned and n is signed ? (i added an comment).

IMHO we should simply add a sentence that "examples are examples and
will not check for every possible error condition."

re,
 wh

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

char *
make_message2 (const char *fmt, ...)
{
  int n = 0;
  size_t size=0;
  char *p = NULL;
  va_list ap;

  /* figure our required size */
  va_start (ap, fmt);
  n = vsnprintf (p, size, fmt, ap);
  va_end (ap);

  if (n < 0)
    return NULL;

  /* size is unsigned */
  size=n;

  /* leave room for \0 */
  size++;
  p = malloc (size);
  if (p == NULL)
    return NULL;

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

return p;
}




> Since this example has been modified by you (Walter Harms), after the
> bug #794947 [1] has been reported, I wanted to ask your opinion on the
> best option.
> 
> Should we add this test to good practice, or rather a comment to mention
> that the case is not taken into account because the example uses glibc?
> 
> Regards,
> 
> [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=794947
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html