Bug 56858 - non-terminated buffer passed to ap_log_rerror
Summary: non-terminated buffer passed to ap_log_rerror
Status: CLOSED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_proxy_fcgi (show other bugs)
Version: 2.5-HEAD
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk, PatchAvailable
Depends on:
Blocks:
 
Reported: 2014-08-15 00:54 UTC by Manuel Mausz
Modified: 2014-12-05 12:29 UTC (History)
0 users



Attachments
stderr-terminate-buffer.patch (726 bytes, patch)
2014-08-15 00:54 UTC, Manuel Mausz
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Mausz 2014-08-15 00:54:55 UTC
Created attachment 31919 [details]
stderr-terminate-buffer.patch

iobuf isn't terminated before passed to ap_log_rerror
Comment 1 Jeff Trawick 2014-08-15 19:02:23 UTC
This for the report.  Here is a simpler fix:

Index: modules/proxy/mod_proxy_fcgi.c
===================================================================
--- modules/proxy/mod_proxy_fcgi.c	(revision 1617253)
+++ modules/proxy/mod_proxy_fcgi.c	(working copy)
@@ -665,7 +665,7 @@
                 /* TODO: Should probably clean up this logging a bit... */
                 if (clen) {
                     ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01071)
-                                  "Got error '%s'", iobuf);
+                                  "Got error '%.*s'", (int)readbuflen, iobuf);
                 }
 
                 if (clen > readbuflen) {

I'll commit that when I get a chance to test...
Comment 2 Manuel Mausz 2014-08-15 19:14:30 UTC
The simpler one was initial patch, but mine is a bit faster :)
Comment 3 Jeff Trawick 2014-08-15 19:28:12 UTC
>The simpler one was initial patch, but mine is a bit faster :)
That seems odd, since it throws away the knowledge of how long the string is.

With .*, the string length doesn't have to be calculated.
Comment 4 Manuel Mausz 2014-08-15 19:34:39 UTC
(In reply to Jeff Trawick from comment #3)
> >The simpler one was initial patch, but mine is a bit faster :)
> That seems odd, since it throws away the knowledge of how long the string is.
> 
> With .*, the string length doesn't have to be calculated.
That's because ap_log_rerror has to walk down the array until either the first 0-byte or the supplied length in order to determine the "real" length of the array. See https://github.com/apache/apr/blob/trunk/strings/apr_snprintf.c#L975

However I'm totally fine with your patch as well. Just wanted to point this out.
Comment 5 Jeff Trawick 2014-08-16 14:24:41 UTC
Thanks for explaining (blush).  We'll go with your original, unposted patch which was the same as mine.
Comment 6 Jeff Trawick 2014-08-16 19:19:33 UTC
The fix is now in httpd trunk with r1618401, and is proposed for backport to the 2.4.x branch.

Thanks!
Comment 7 Christophe JAILLET 2014-08-18 06:23:26 UTC
BTW,

in the comment pointed out by Manuel, in the string "My reading is is precision is specified and", shouldn't "is is" be "is if" ?
Comment 8 Jeff Trawick 2014-12-05 12:29:05 UTC
This is in the 2.4.x branch and will be in the next release.