Bug#794947: manpages-dev: printf(3) example: possible integer overflow
On Thu, Feb 18, 2016 at 08:18:33PM +0100, Stéphane Aulery wrote:
> Le 17/02/2016 22:13, walter harms a écrit :
> > >
> > > 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):
> > > > [...]
> >
> > 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).
> > [...]
> [...]
> So I will do a patch with your new corrected version that is very readable.
JFTR, that example was then changed significantly upstream between 3.82
and 4.04 (the latter being the first version that entered Debian after
3.74), saw a minor update in 4.10, and was adjusted again in 5.07 with
then "size" being of type "size_t". (With some non-code changes
sprinkled inbetween.)
Upstream 5.13 now lists
| To allocate a sufficiently large string and print into it (code correct
| for both glibc 2.0 and glibc 2.1):
|
| #include
| #include
| #include
|
| char *
| make_message(const char *fmt, ...)
| {
| int n = 0;
| size_t size = 0;
| char *p = NULL;
| va_list ap;
|
| /* Determine required size. */
|
| va_start(ap, fmt);
| n = vsnprintf(p, size, fmt, ap);
| va_end(ap);
|
| if (n < 0)
| return NULL;
|
| size = (size_t) n + 1; /* One extra byte for '\0' */
| p = malloc(size);
| if (p == NULL)
| return NULL;
|
| va_start(ap, fmt);
| n = vsnprintf(p, size, fmt, ap);
| va_end(ap);
|
| if (n < 0) {
| free(p);
| return NULL;
| }
|
| return p;
| }
which is an extension of what walter harms proposed in
.
As such I think this bug here could just be closed.
Cheers,
Flo
signature.asc
Description: PGP signature
Bug#794947: manpages-dev: printf(3) example: possible integer overflow
Hello Walter,
Le 17/02/2016 22:13, walter harms a écrit :
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".
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."
I agree with the general idea: the examples must remain so. They must
also be correct. Tough choice!
I will not put a note on this page about it, nor on the other, too much
for so little.
man-pages.7 specifically requests:
Example programs shoulds be fairly short (preferably less than 100
lines;
Ideally less than 50 lines).
Example programs shoulds do error checking after-system calls and
library function calls.
So I will do a patch with your new corrected version that is very readable.
Thanks a lot for your help.
Regards,
--
Stéphane Aulery
Bug#794947: manpages-dev: printf(3) example: possible integer overflow
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-...@vger.kernel.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
#include
#include
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
>
Bug#794947: manpages-dev: printf(3) example: possible integer overflow
retitle 794947 printf(3): possible integer overflow in make_message
example
severity 794947 wishlist
tags 794947 + confirmed
tags 794947 + upstream
forwarded 794947 linux-...@vger.kernel.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".
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
--
Stéphane Aulery
Bug#794947: manpages-dev: printf(3) example: possible integer overflow
Package: manpages-dev
Version: 3.74-1
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.
--
Jakub Wilk
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
