Bug 23282 - vsnprintf(3) example promotes code which ignores error return code
vsnprintf(3) example promotes code which ignores error return code
Status: RESOLVED CODE_FIX
Product: Documentation
Classification: Unclassified
Component: man-pages
All Linux
: P1 normal
Assigned To: documentation_man-pages@kernel-bugs.osdl.org
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-19 02:58 UTC by Graham Gower
Modified: 2013-02-28 13:20 UTC (History)
2 users (show)

See Also:
Kernel Version:
Tree: Mainline
Regression: No


Attachments
handle the error code (791 bytes, patch)
2013-02-08 09:28 UTC, Marshel Abraham
Details | Diff
handle error code (1.96 KB, application/octet-stream)
2013-02-11 15:09 UTC, Marshel Abraham
Details
handle error code: patch no.3 (1.55 KB, application/octet-stream)
2013-02-25 16:42 UTC, Marshel Abraham
Details

Description Graham Gower 2010-11-19 02:58:03 UTC
The example given in the vsnprintf(3) man page (release 3.31) attempts to be backwards compatible with glibc < 2.0.6 by assuming that a negative return code from vsnprintf indicates truncation.

If a negative return code is indicated for other reasons, the example will loop until the process' virtual memory is exhausted.

Please see the following for an example of how a malicious user could deliberately trigger this (potentially causing a denial of service).
http://my.opera.com/taviso/blog/2007/05/28/auditing-puzzle
Comment 1 Marshel Abraham 2013-02-08 09:28:03 UTC
Created attachment 92661 [details]
handle the error code

The above patch tries to be completely in align with the coding style.This change in example will prevent the crashing of code.
Comment 2 Graham Gower 2013-02-09 00:16:38 UTC
Marshel, that patch does not address the ambiguity created by backwards compatibility with glibc 2.0.6, it merely removes the backwards compatilibility (while retaining notes that it is compatible). You also omit the return value in your return statement.

I'd be happy with the backwards compatibility removed (and a note to that effect), given glibc 2.0 is now 15 years old. The code could then be simplified. At the time I encountered this issue, I wrote the following code, which would just error out in the case of truncation with old glibc. http://code.google.com/p/opkg/source/browse/trunk/libopkg/sprintf_alloc.c
Comment 3 Michael Kerrisk 2013-02-09 02:08:38 UTC
 (In reply to comment #2)

> I'd be happy with the backwards compatibility removed (and a note to that
> effect), given glibc 2.0 is now 15 years old. The code could then be
> simplified. 

Agreed. I'd accept a patch along those lines. Does someone want to write one?
Comment 4 Marshel Abraham 2013-02-11 15:09:28 UTC
Created attachment 93121 [details]
handle error code 

Okay this time I have tried to address the backward compatibility issue.I have made the change in NOTES section of the Documentation.I have gone through the entire Documentation and could find only two places where it is mentioned.

Regarding the return value I couldn't figure out what to return :-) apart from NULL.

I have kept the code simple and also test it.Though the codes have been changed comments are not felt like they fit perfectly.
Comment 5 Marshel Abraham 2013-02-11 15:11:10 UTC
Guys, Thanks a ton for your reply.I have added another patch. Waiting for your response.
Comment 6 Michael Kerrisk 2013-02-11 21:38:50 UTC
Graham,

Could you review?

Thanks,

Michael
Comment 7 Graham Gower 2013-02-12 07:11:35 UTC
I'm not able to properly look at it this week... Due to connectivity problems I'm stuck to typing this on my phone.
Comment 8 Marshel Abraham 2013-02-19 07:08:07 UTC
Just to remind you that the patch have not been reviewed. Are you back to Good Internet...?
Comment 9 Marshel Abraham 2013-02-22 11:28:29 UTC
Can Somebody review the patch...?
Comment 10 Marshel Abraham 2013-02-22 17:23:36 UTC
(In reply to comment #6)
> Graham,
> 
> Could you review?
> 
> Thanks,
> 
> Michael


Michael,

Can I expect some response...?
Comment 11 Graham Gower 2013-02-22 23:01:24 UTC
Marshal, I don't apreciate being constantly badgered to review your patch. *One* email, or *one* comment here would have sufficed.


>--- a/man3/printf.3
>+++ b/man3/printf.3
>@@ -892,7 +892,9 @@ and
> conforms to the C99 standard, that is, behaves as described above,
> since glibc version 2.1.
> Until glibc 2.0.6 they would return \-1
>-when the output was truncated.
>+when the output was truncated.In current implementation a negative value 
>+is returned if an output error is encountered(see Return Value).Hence
>+these functions are not compatible with glibc 2.0.6 and before.

I'm unfamiliar with man page formatting conventions, but a cursory look at the rest of this file indicates that new sentences should be on new lines. In any event, I don't think this text adds anything useful.

> .\" .SH HISTORY
> .\" UNIX V7 defines the three routines
> .\" .BR printf (),
>@@ -1024,7 +1026,7 @@ With the value:
> one might obtain "Sonntag, 3. Juli, 10:02".
> .PP
> To allocate a sufficiently large string and print into it
>-(code correct for both glibc 2.0 and glibc 2.1):
>+(code incompatible for glibc 2.0.6 and before):

How about
"If truncation occurs in glibc versions prior to 2.0.6, this is treated as an error instead of being handled gracefully"

> .nf
> 
> #include <stdio.h>
>@@ -1050,18 +1052,21 @@ make_message(const char *fmt, ...)
>         n = vsnprintf(p, size, fmt, ap);
>         va_end(ap);
> 
>+	/* Check error code */
>+	
>+	if (n < 0)
>+	    return NULL ;
>+	     

The indentation here is not consistent with the rest of the example.

>         /* If that worked, return the string. */
> 
>-        if (n > \-1 && n < size)
>+        if (n < size)
>             return p;
> 
>         /* Else try again with more space. */
> 
>-        if (n > \-1)    /* glibc 2.1 */
>+        else
>             size = n+1; /* precisely what is needed */
>-        else           /* glibc 2.0 */
>-            size *= 2;  /* twice the old size */
>-
>+        
>         if ((np = realloc (p, size)) == NULL) {
>             free(p);
>             return NULL;
>-- 
>1.7.0.4
Comment 12 Michael Kerrisk 2013-02-25 06:44:02 UTC
@Graham: Thanks for the review Graham.

@Marshel: Thanks for the revised patch. I appreciate that you're keen to get your patch reviewed, but everyone's on private time here, so things can be slow. As such, it's best to space your pings out a bit. I'm inclined to agree with Graham's comment that the first piece of text that you added probably isn't needed. But, otherwise, if you can fix the rest of the patch up as per Graham's comments, I'm happy to apply it.
Comment 13 Marshel Abraham 2013-02-25 16:42:46 UTC
Created attachment 94071 [details]
handle error code: patch no.3

*One Comment* :-)

Sorry for the trouble and Thanks for your patience. I think indentation is right this time.

Please review and let me know about further changes needed.
Comment 14 Graham Gower 2013-02-25 22:04:31 UTC
acked-by: graham.gower@gmail.com
Comment 15 Michael Kerrisk 2013-02-28 13:20:21 UTC
Applied.

Marshel, thanks for the fix to Graham's bug!

Note You need to log in before you can comment on or make changes to this bug.