[Bug 57045] dumpio output strings are missing

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

Christophe JAILLET <***@wanadoo.fr> changed:

What |Removed |Added
----------------------------------------------------------------------------
OS| |All

--- Comment #1 from Christophe JAILLET <***@wanadoo.fr> ---
This is maybe related to PR54179.

This has been fixed in 2.4.4.

Could you test with a newer version to see if your issue is solved?

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #2 from Kurtis Rader <***@skepticism.us> ---
The problem is that null (i.e., zero) bytes are dropped because the logging
code treats all data as null-terminated strings. See my blog post at
http://www.skepticism.us/2015/09/apache-module-dumpio-doesnt-dump-null-zero-bytes/.
I have a fix for this and will work to get it integrated into the project.
You'll find the fixed source for the mod_dumpio.c file linked from my post
where I announced the availability of the fix:
http://www.skepticism.us/2015/10/a-fix-for-the-apache-mod_dumpio-module-not-dumping-null-bytes/

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

Kurtis Rader <***@skepticism.us> changed:

What |Removed |Added
----------------------------------------------------------------------------
Hardware|PC |All
Version|2.4.2 |2.4.16
Keywords| |PatchAvailable, PortBack

--- Comment #3 from Kurtis Rader <***@skepticism.us> ---
Also, I changed the version to 2.4.16 since the bug exists in all versions up
to and including the most recent production release. I also changed the
hardware to "all" because the bug is independent of the hardware platform.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #4 from Kurtis Rader <***@skepticism.us> ---
Created attachment 33162
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33162&action=edit
fixed mod_dumpio.c module

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #5 from Kurtis Rader <***@skepticism.us> ---
Created attachment 33163
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33163&action=edit
Patch against 2.4.16

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #6 from Kurtis Rader <***@skepticism.us> ---
Created attachment 33164
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33164&action=edit
Program to decode output of fixed mod_dumpio

This Python program extracts the data from the error log lines written by
mod_dumpio and decodes the logged data to reconstruct the original data. Below
is an example from my error log. The "=#" lines are included to clearly
delineate the reconstructed data from the lines in the error log (both sets of
lines are written to stdout).

=#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#=
GET /wp-admin/admin-ajax.php?action=revslider_show_image&img=..%2Fwp-config.php
HTTP/1.1
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101
Firefox/31.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Encoding: gzip, deflate
Accept-Language: en-US,en;q=0.5
Host: skepticism.us
Content-Length: 0

=#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#==#=

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #7 from Yann Ylavic <***@gmail.com> ---
(In reply to Kurtis Rader from comment #5)

Created attachment 33163 [details]
Patch against 2.4.16

This patch looks not thread-safe (static encoded_{buf,idx} and space_left can't
be accessed concurrently).

How about using ap_log_[r]data() introduced in 2.4.11?

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

This patch looks not thread-safe...

It's definitely not thread-safe. I should have mentioned that this version only
works reliably with MPM pre-fork; which is what I'm currently using since my
site runs WordPress. I threw this together over a couple of evenings and wanted
to get it out there for feedback. Assuming no one has any other problems with
the code I'll add the necessary support for thread local data to make it
thread-safe.

How about using ap_log_[r]data() introduced in 2.4.11?

That looks like it will emit data in a non-backward compatible format. It would
also mean the code can't be used with 2.2.x. For this change I'd prefer to be
conservative and keep the logged format unchanged to the extent possible and
remain compatible with 2.2.x. Obviously I'm changing how 0xff is represented in
the error log but it's a safe bet no one will care because they've obviously
never tried to use the binary data that is logged.


P.S., I looked at the existing uses of ap_log_rdata() in the 2.4.16 tree and
they all appear to be incorrect. The ap_log_rdata function declaration doesn't
match any of the uses. Consider the sole use in modules/proxy/mod_proxy_fcgi.c:

ap_log_rdata(APLOG_MARK, APLOG_TRACE8, r, "FastCGI header",
farray, AP_FCGI_HEADER_LEN, 0);

The only reason this compiles is because "r" is defined at function scope. If
you rename "r" to "req" throughout the function it fails to compile with this
error:

mod_proxy_fcgi.c:577:13: error: use of undeclared identifier 'r'
ap_log_rdata(APLOG_MARK, APLOG_TRACE8, req, "FastCGI header",
^
/Users/krader/brew-httpd-2.4.16/include/http_log.h:636:27: note: expanded from
macro 'ap_log_rdata'


P.P.S., That magic zero as the last parameter of the call rather than
AP_LOG_DATA_DEFAULT is gross.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

Post by b***@apache.org
mod_proxy_fcgi.c:577:13: error: use of undeclared identifier 'r'
ap_log_rdata(APLOG_MARK, APLOG_TRACE8, req, "FastCGI header",
^
/Users/krader/brew-httpd-2.4.16/include/http_log.h:636:27: note: expanded
from macro 'ap_log_rdata'

fixed in trunk / proposed for backport

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

Kurtis Rader <***@skepticism.us> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@skepticism.us
Attachment #33162|0 |1
is obsolete| |

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

Kurtis Rader <***@skepticism.us> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #33163|0 |1
is obsolete| |

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

Kurtis Rader <***@skepticism.us> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #33164|0 |1
is obsolete| |

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #10 from Kurtis Rader <***@skepticism.us> ---
Created attachment 33179
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33179&action=edit
Patch against 2.4.16

This version, unlike my first submission, is thread-safe. I've tested this on
Mac OS X and Ubuntu with both input and output logging enabled.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #11 from Kurtis Rader <***@skepticism.us> ---
Created attachment 33180
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33180&action=edit
fixed mod_dumpio.c module

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #12 from Kurtis Rader <***@skepticism.us> ---
Created attachment 33181
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33181&action=edit
Program to decode output of fixed mod_dumpio

This is an updated version of the program I posted earlier. This version takes
advantage of the "new request" log message I introduced in the latest version
of my mod_dumpio patch.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #13 from Eric Covener <***@gmail.com> ---
Comment on attachment 33180
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33180
fixed mod_dumpio.c module

Can you please post changes as diff -u output?

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #14 from Kurtis Rader <***@skepticism.us> ---
"Can you please post changes as diff -u output?"

I did. See the first attachment:
https://bz.apache.org/bugzilla/attachment.cgi?id=33179

I posted the entire module to make it easy for someone to see the entirety of
the change since the patch is twice the length of the original module.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #15 from Eric Covener <***@gmail.com> ---
(In reply to Kurtis Rader from comment #14)

Post by b***@apache.org
"Can you please post changes as diff -u output?"
https://bz.apache.org/bugzilla/attachment.cgi?id=33179
I posted the entire module to make it easy for someone to see the entirety
of the change since the patch is twice the length of the original module.

Sorry, assumed it superceded.

It looks like a large part of the diff (other than that there's info for 2
copies of the mod?) is refactoring and changing C comments to C++/c99 comments.
Noise aside, the latter should be avoided for modules. The
refactoring/all-in-one patch makes it a lot of labor to review.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

Post by b***@apache.org
It looks like a large part of the diff (other than that there's
info for 2 copies of the mod?) is refactoring and changing
C comments to C++/c99 comments. Noise aside, the latter should
be avoided for modules.

I wholly agree. However, in this case the only gratuitous comment style change
I made was the comments in the AP_DECLARE_MODULE stanza at the end of the file.
I'm happy to revert that portion of the patch. There are a couple of other
places where I changed the comment style but it doesn't materially affect the
size of the diff because I modified the comment as a consequence of changing
the actual logic. The diff is huge because of the significant rewrite of the
core dumpit() function.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #17 from Kurtis Rader <***@skepticism.us> ---
Ping? I got tired of building httpd from source and am now relying on my distro
which currently provides httpd 2.4.29. It's a trifle annoying that this still
isn't fixed. Especially since I provided a patch that fixed the problem and
other than some grumbling about changing some comments to use "//" style
comments I didn't get any constructive feedback on my patch. As I wrote in a
previous comment "The diff is huge because of the significant rewrite of the
core dumpit() function." And that's because the original implementation does
not work as advertised.

[b***@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=57045

--- Comment #18 from Kurtis Rader <***@skepticism.us> ---
I've given up on Apache+Wordpress. In part because issues like this are not
resolved in a timely manner. This issue has been open four years. I submitted a
fix three years ago. If my fix is not acceptable it would be nice to know why
not. I've switched to Caddy+Hugo for my blog. Setting up the Caddy web server
was trivial and immediately gave me a HTTPS enabled site. Subsequent changes to
my Caddy web server config was far simpler than any change I made to my Apache
web server config.

I appreciate that this project has to support a lot of baggage from earlier
releases in order to avoid gratuitously breaking existing uses. But fixing this
issue by incorporating my change wouldn't break any existing use other than the
most pathological. That this is still broken tells me the project is dead.