httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject mod_proxy http request body code review request
Date Thu, 08 Sep 2005 22:55:39 GMT
As many of the proxy reviewers could not follow
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230744&r1=230703&r2=230744

I've provided the following fork of that codebase, to try to repair the
damage from the vetoed 171205 commit, in a piece by piece analysis of
what's changed and why.

The new code is only marginally different (in expected ways, based on
our API changes) from the trunk code, which I'm certain that our beta
proponents have actively tested.  The resulting difference from trunk is

svn diff \
 
http://svn.apache.org/repos/asf/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c

\
 
http://svn.apache.org/repos/asf/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c

Please test, and vote on either the entire patch or nak specific commits
below with technical justification, so that we might release 2.0.55 and
avoid backing out 171205 once again.  AIUI, JimJ and myself are +1 so
far, which are all the votes collected (I'd addressed several -1's to
address the technical concerns within the fixes below).  To those who
take the time to review, Thank You!

Log of /httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c

No default branch
Bookmark a link to HEAD: (view) (download) (as text)
Revision 230744 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 03:33:20 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 65895 byte(s)
Diff to previous 230741 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230744&r1=230741&r2=230744

   The final, inconsequential patch which makes the diff for request
   body processing between trunk/ and 2.1.x/ very easy to read.

   This patch consists of nothing except whitespace changes, and the
   change from b to bb for the local bucket brigade variable name,
   to disambiguate it from a bucket.

Revision 230741 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 03:20:27 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 65742 byte(s)
Diff to previous 230738 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230741&r1=230738&r2=230741

   Fix some very minor nits in ap_proxy_http_request() which make it
   quite simple to diff to the current trunk/ code, ensuring the most
   accurate review possible.

   Consists only of whitespace and line spacing changes, and a change
   from 'bb' to 'header_brigade' for this local argument variable's name,
   so there are no functional changes here.

Revision 230738 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 03:10:10 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 65595 byte(s)
Diff to previous 230737 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230738&r1=230737&r2=230738

   Sync to trunk/, add an extra measure of paranoia to the cl + te case.

Revision 230737 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 03:03:56 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 65510 byte(s)
Diff to previous 230736 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230737&r1=230736&r2=230737

   Commit a comment from trunk/, this brings spool_reqbody_cl in sync
   with trunk/.

Revision 230736 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 03:01:39 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 65446 byte(s)
Diff to previous 230729 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230736&r1=230729&r2=230736

   Backport r230735, we need not look at the first bucket for EOS,
   because the outer while loop protected us from that case.

   Backport the header brigade changes as it's impossible to have
   a body request waiting for a final send.  Look at seen_eos to
   flush us in the request body loop, and handle the only exception,
   (header_brigade), outside of that loop.

   This brings stream_reqbody_cl in sync with the trunk.

Revision 230729 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 01:44:46 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 65751 byte(s)
Diff to previous 230728 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230729&r1=230728&r2=230729

   Backport the fix to handling Connection: close.  The existing code
   was impossible to follow; the new code sets up p_conn->close correctly,
   and uses that evaluated value to inject the appropriate choice 
immediately
   before passing the request to the backend server.

Revision 230728 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 01:39:14 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 65459 byte(s)
Diff to previous 230727 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230728&r1=230727&r2=230728

   Backport the corrected C-L streamed, v.s. spooled, v.s. T-E: chunked
   selection logic from httpd trunk/.  This now correctly chooses the
   most portable mechanism (e.g. C-L) when we can see the entire body,
   even for chunked bodies from the client, falls back on spool cl when
   it's necessary, and uses chunked when we have faith in it.

Revision 230727 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 01:31:02 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 64590 byte(s)
Diff to previous 230726 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230727&r1=230726&r2=230727

   Backport the rejection of non-'chunked' values in the transfer
   encoding; we simply don't know what, exactly to do with them.

   Backport the reporting of 'both C-L and T-E' when we encounter
   this edge case, setting the connection up to close down once
   we finish (perhaps we were victims of a request splitting attack).

Revision 230726 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 01:28:14 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 63526 byte(s)
Diff to previous 230725 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230726&r1=230725&r2=230726

   Backport the fix to an edge case, it's now possible for a primary
   request which has a body in spite of what was determined by the
   header parsing; this would usually be due to an input filter between
   the client request and mod_proxy.  Add another consideration, and
   force the C-L determination if we saw bytes in already.

Revision 230725 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 01:26:34 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 63460 byte(s)
Diff to previous 230724 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230725&r1=230724&r2=230725

   Backport the fix to an edge case; it's now much more efficient to
   entirely skip request body determinations for subrequests, stuff
   in an EOS and we are off to the races, ready to create a body-less
   proxied subrequest.

Revision 230724 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 01:22:17 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 63218 byte(s)
Diff to previous 230722 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230724&r1=230722&r2=230724

   Backport the body pre-fetch code from trunk/, modifying the three
   stream_te / stream_cl / spool_cl functions to presume an input_brigade
   before fighting with fetching additional body content.

   We will be using the bytes_read in a later commit to make a better
   decision about choosing to send a C-L or chunked body to the backend.

Revision 230722 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 01:10:20 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 61915 byte(s)
Diff to previous 230720 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230722&r1=230720&r2=230722

   Backport yet another security fix; if stream_cl exceeds the 'stated'
   CL which proxy_request_body asked us to send, then we have to quit
   forwarding any more bytes (we won't even pass the header if we
   hadn't yet.)

   Closes an HTTP Request splitting edge case.

Revision 230720 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 01:03:09 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 61005 byte(s)
Diff to previous 230719 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230720&r1=230719&r2=230720

   Fix a buglett in backport 230718; get our selection of brigades correct.

   (tis what happens when our code is this far out-of-sync with trunk, 
sorry.)

Revision 230719 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 00:59:40 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 61013 byte(s)
Diff to previous 230716 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230719&r1=230716&r2=230719

   Backport fix r230718; a soon-to-be impossible edge case; we are always
   sending a body (even an 'empty' body) using stream_chunked, so we must
   always send the trailing "0" end of body marker.

Revision 230716 - (view) (download) (as text) - [select for diffs]
Modified Mon Aug 8 00:30:27 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 61036 byte(s)
Diff to previous 230714 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230716&r1=230714&r2=230716

   Finish up the header brigades up-front; we simply know ahead of time if
   they are necessary or not.  This changes one behavior; the stream_chunked
   now always sends a transfer encoded body; even if it's nothing but an
   empty body.

   Also, Fix a bugglet in backporting the input_brigade argument; need the
   bucket_alloc already.

Revision 230714 - (view) (download) (as text) - [select for diffs]
Modified Sun Aug 7 23:45:54 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 61086 byte(s)
Diff to previous 230712 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230714&r1=230712&r2=230714

   Pre-commit a simple bit from r219224, pass an initialized input_brigade
   off to the spool/stream functions.  Effectively a no-op so far, but in
   a couple of patches, the reason becomes obvious.

Revision 230712 - (view) (download) (as text) - [select for diffs]
Modified Sun Aug 7 23:32:36 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 60767 byte(s)
Diff to previous 230711 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230712&r1=230711&r2=230712

   Backport a logging change to make things clearer; includes jorton's 
r224721 fix.

Revision 230711 - (view) (download) (as text) - [select for diffs]
Modified Sun Aug 7 23:23:38 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 60629 byte(s)
Diff to previous 230709 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230711&r1=230709&r2=230711

   Backport part of r216156 and very minor bits of r218978;

   Collapse the handling of Content-Length / Transfer-Encoding header
   values into the headers loop saving a bit of cpu and making the
   results clearer, handle the (r->main) subrequest case a bit more
   efficiently as well, cutting out some duplicate tests, and a mild
   whitespace issue in the If-Unmodified-Since test line.

   Fixes a subrequest bug where subrequests *still* would attempt to
   read Transfer-Encoding: chunked request bodies.

Revision 230709 - (view) (download) (as text) - [select for diffs]
Modified Sun Aug 7 22:55:37 2005 UTC (4 weeks, 3 days ago) by wrowe
File length: 60932 byte(s)
Diff to previous 230708 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230709&r1=230708&r2=230709

   Backport part of r216156;

   send_request_body makes it very difficult to follow all the mistakes
   in this code.  Fold send_request_body into ap_proxy_http_request
   so that proxy_http_request makes all of the elections.


Revision 230708 - (view) (download) (as text) - [select for diffs]
Modified Sun Aug 7 22:22:41 2005 UTC (4 weeks, 4 days ago) by wrowe
File length: 61447 byte(s)
Diff to previous 230706 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230708&r1=230706&r2=230708

   End apr_natstrcasecmp, backporting part of r216111 (there may remain
   other apr_natstrcasecmp abuse in other files).

   Still unsure if apr_strnatcasecmp of the hostname is deliberate to
   handle leading 0's in ip addresses, so leaving that single use case.

Revision 230706 - (view) (download) (as text) - [select for diffs]
Modified Sun Aug 7 22:00:54 2005 UTC (4 weeks, 4 days ago) by wrowe
File length: 61585 byte(s)
Diff to previous 230704 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230704&r1=230706&r2=230704

   Now backport whitespace-only changes from r209836, making the rest of
   the patches more legible, and the resulting code comparable   to 
httpd/trunk/modules/proxy/mod_proxy_http.c.

Revision 230704 - (view) (download) (as text) - [select for diffs]
Modified Sun Aug 7 21:57:56 2005 UTC (4 weeks, 4 days ago) by wrowe
File length: 61871 byte(s)
Diff to previous 230703 (colored)
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230704&r1=230703&r2=230704

   Prepare to backport r209836 whitespace changes; this is the one
   minor code change to declare *buf once, obliterating the need for
   a number of code blocks throughout this function.

Revision 230703 - (view) (download) (as text) - [select for diffs]
Modified Sun Aug 7 21:09:38 2005 UTC (4 weeks, 4 days ago) by wrowe
File length: 61970 byte(s)
Diff to previous 219059 (colored)

   Create proxy-reqbody patch evaluation branch.


Mime
View raw message