Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: domain of minfrin@sharp.fm designates
 72.32.122.47 as permitted sender)
Message-ID: <48D52F8E.8070203@sharp.fm>
Date: Sat, 20 Sep 2008 19:14:54 +0200
From: Graham Leggett <minfrin@sharp.fm>
User-Agent: Thunderbird 2.0.0.16 (Macintosh/20080707)
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: svn commit: r592951 - in /httpd/httpd/trunk: ./ docs/manual/mod/
 include/ modules/filters/ modules/http/ server/
References: <20071107233112.524B91A9832@eris.apache.org>
 <20071201175055.73b07982@grimnir> <475205FA.90307@sharp.fm>
 <47FE8C3B.7040309@apache.org> <48D48991.1050306@force-elite.com>
In-Reply-To: <48D48991.1050306@force-elite.com>
Content-Type: multipart/signed; protocol="application/x-pkcs7-signature";
 micalg=sha1; boundary="------------ms080407050106020501070902"

This is a cryptographically signed message in MIME format.

--------------ms080407050106020501070902
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Paul Querna wrote:

> +1, this revision r592951, and the derivate r647263 need to be 
> rethought.  IMO is that this doesn't belong in the reqeust_rec.
> 
> I'm only realizing this went in 10 months after the first commit, and 5 
> months after it was rewritten in r647263, a sign of my inactivity in 
> httpd land, so I'm not gonna 'veto' it, but damn I really don't like 
> having this stuff on the reqeust_rec.

Having had the time to think this through, to be honest I cannot think 
of a different place for it to go other than request_rec.

At the core of the problem is a flaw that has been repeated over and 
over inside lots of modules within the server, and which this code 
partially fixes.

The flaw is that by design, the core will allow a request to spawn a 
subrequest, but the core offers no functionality for the subrequest to 
read a request body.

So you get the same repeated code, over and over again in various 
modules, incarnations of this:

/* what we have now */
if ( ! we_are_a_subrequest) {
    read_request_body();
}
else {
    ignore_body_somehow();
}

Now whether it is relevant for a subrequest to read a request body isn't 
important, what's important is that the subrequest isn't even allowed to 
try, and the subrequest has to go out of its way to make sure it doesn't 
read from the network twice. The subrequest shouldn't have to care about 
this stuff.

With the kept_body code, two new possibilities are available that have 
not been available before:

- Where the admin wants to set aside the body to be re-read by 
subrequests, the kept_body filter makes this possible, by allowing a 
kept body to be reread by a subrequest safely. The subrequest now 
becomes a fully fledged subrequest, able to access everything the 
request can.

- Where the admin doesn't want to set aside the body, the kept_body 
filter can be inserted anyway, with an empty brigade. This "caps" the 
input filter stack, guaranteeing that the network can never be read 
twice, regardless of how many times it tries to read from the network.

These two possibilities together mean that any module is now free to try 
and read a request body if it wants to, and the kept_body filter 
guarantees that the underlying network is never read from twice.

This in turn means that all the different variations of "detect 
subrequest and try to to read the body a second time" that are scattered 
around the code become obsolete and can be removed, resulting in a much 
simpler contract for module code, and a significantly cleaner server:

/* a cleaner alternative */
read_request_body();

In order for the partial fix to become a full fix, the kept_body filter 
should be unconditionally added to all subrequests (with an empty 
brigade in the simplest case), and all the instances of the "if ( ! 
we_are_a_subrequest)" code should be collapsed and removed, and modules 
should be free to try and read the body as many times as they like, safe 
in the knowledge that the kept_body filter guarantees the network will 
only ever be read from once.

Regards,
Graham
--

--------------ms080407050106020501070902
Content-Type: application/x-pkcs7-signature; name="smime.p7s"
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="smime.p7s"
Content-Description: S/MIME Cryptographic Signature

MIAGCSqGSIb3DQEHAqCAMIACAQExCzAJBgUrDgMCGgUAMIAGCSqGSIb3DQEHAQAAoIIJNTCC
AvUwggJeoAMCAQICEAxljuBYvPVR3WhH6nkbTPowDQYJKoZIhvcNAQEFBQAwYjELMAkGA1UE
BhMCWkExJTAjBgNVBAoTHFRoYXd0ZSBDb25zdWx0aW5nIChQdHkpIEx0ZC4xLDAqBgNVBAMT
I1RoYXd0ZSBQZXJzb25hbCBGcmVlbWFpbCBJc3N1aW5nIENBMB4XDTA3MTAxNDEyMzQyMVoX
DTA4MTAxMzEyMzQyMVowXTEQMA4GA1UEBBMHTGVnZ2V0dDEPMA0GA1UEKhMGR3JhaGFtMRcw
FQYDVQQDEw5HcmFoYW0gTGVnZ2V0dDEfMB0GCSqGSIb3DQEJARYQbWluZnJpbkBzaGFycC5m
bTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALLUMqDEkhgiT7ePdkErBVE2tND+
6C8ElWEOaCSdbAlvwUXgKFzIR7zaD6c1CS2czmBslMwEb3LJHPWjgPN497wSERghkeAa+Fyw
WqydJr+6WE1G67wHsg67tmGPAxG0Lf6eKpsiyh+u0ojKk4n0mRZ6HQxu6PqZYzJ2vOrT4gYz
uVlz4O8TRhHOXGKqclCTxVOfEQMS3AmKDkdkNKJxgkrXSCDZ3mWs1K7yuZ6f0/30Z6AvseTF
N7CWPD7uuf5TvaVd5luOYiprUTl+u+0+CHjG4uiug54FZAyID5N7cMJy6iBRHPfLk4cU+Ksi
R99WkanPHb9wVQ2F34S+7yGdwqECAwEAAaMtMCswGwYDVR0RBBQwEoEQbWluZnJpbkBzaGFy
cC5mbTAMBgNVHRMBAf8EAjAAMA0GCSqGSIb3DQEBBQUAA4GBAB76lZ2i2DDcVPlrcDWPrGIk
mcZSoWm/7HLRvws4+jbZ++bczzUruhm1t410Z8oj95sU5pQ83SoGS3RXAn/+TX0cpgNtx4Sw
J+Nfhvey2w1TE/NLlN3n7q0m7Bm4j4+zNKXLjFj6B30Ifce8qHw7l69MSVcKoJiyd8EMM4q8
Dm+wMIIC9TCCAl6gAwIBAgIQDGWO4Fi89VHdaEfqeRtM+jANBgkqhkiG9w0BAQUFADBiMQsw
CQYDVQQGEwJaQTElMCMGA1UEChMcVGhhd3RlIENvbnN1bHRpbmcgKFB0eSkgTHRkLjEsMCoG
A1UEAxMjVGhhd3RlIFBlcnNvbmFsIEZyZWVtYWlsIElzc3VpbmcgQ0EwHhcNMDcxMDE0MTIz
NDIxWhcNMDgxMDEzMTIzNDIxWjBdMRAwDgYDVQQEEwdMZWdnZXR0MQ8wDQYDVQQqEwZHcmFo
YW0xFzAVBgNVBAMTDkdyYWhhbSBMZWdnZXR0MR8wHQYJKoZIhvcNAQkBFhBtaW5mcmluQHNo
YXJwLmZtMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAstQyoMSSGCJPt492QSsF
UTa00P7oLwSVYQ5oJJ1sCW/BReAoXMhHvNoPpzUJLZzOYGyUzARvcskc9aOA83j3vBIRGCGR
4Br4XLBarJ0mv7pYTUbrvAeyDru2YY8DEbQt/p4qmyLKH67SiMqTifSZFnodDG7o+pljMna8
6tPiBjO5WXPg7xNGEc5cYqpyUJPFU58RAxLcCYoOR2Q0onGCStdIINneZazUrvK5np/T/fRn
oC+x5MU3sJY8Pu65/lO9pV3mW45iKmtROX677T4IeMbi6K6DngVkDIgPk3twwnLqIFEc98uT
hxT4qyJH31aRqc8dv3BVDYXfhL7vIZ3CoQIDAQABoy0wKzAbBgNVHREEFDASgRBtaW5mcmlu
QHNoYXJwLmZtMAwGA1UdEwEB/wQCMAAwDQYJKoZIhvcNAQEFBQADgYEAHvqVnaLYMNxU+Wtw
NY+sYiSZxlKhab/sctG/Czj6Ntn75tzPNSu6GbW3jXRnyiP3mxTmlDzdKgZLdFcCf/5NfRym
A23HhLAn41+G97LbDVMT80uU3efurSbsGbiPj7M0pcuMWPoHfQh9x7yofDuXr0xJVwqgmLJ3
wQwzirwOb7AwggM/MIICqKADAgECAgENMA0GCSqGSIb3DQEBBQUAMIHRMQswCQYDVQQGEwJa
QTEVMBMGA1UECBMMV2VzdGVybiBDYXBlMRIwEAYDVQQHEwlDYXBlIFRvd24xGjAYBgNVBAoT
EVRoYXd0ZSBDb25zdWx0aW5nMSgwJgYDVQQLEx9DZXJ0aWZpY2F0aW9uIFNlcnZpY2VzIERp
dmlzaW9uMSQwIgYDVQQDExtUaGF3dGUgUGVyc29uYWwgRnJlZW1haWwgQ0ExKzApBgkqhkiG
9w0BCQEWHHBlcnNvbmFsLWZyZWVtYWlsQHRoYXd0ZS5jb20wHhcNMDMwNzE3MDAwMDAwWhcN
MTMwNzE2MjM1OTU5WjBiMQswCQYDVQQGEwJaQTElMCMGA1UEChMcVGhhd3RlIENvbnN1bHRp
bmcgKFB0eSkgTHRkLjEsMCoGA1UEAxMjVGhhd3RlIFBlcnNvbmFsIEZyZWVtYWlsIElzc3Vp
bmcgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMSmPFVzVftOucqZWh5owHUEcJ3f
6f+jHuy9zfVb8hp2vX8MOmHyv1HOAdTlUAow1wJjWiyJFXCO3cnwK4Vaqj9xVsuvPAsH5/Ef
kTYkKhPPK9Xzgnc9A74r/rsYPge/QIACZNenprufZdHFKlSFD0gEf6e20TxhBEAeZBlyYLf7
AgMBAAGjgZQwgZEwEgYDVR0TAQH/BAgwBgEB/wIBADBDBgNVHR8EPDA6MDigNqA0hjJodHRw
Oi8vY3JsLnRoYXd0ZS5jb20vVGhhd3RlUGVyc29uYWxGcmVlbWFpbENBLmNybDALBgNVHQ8E
BAMCAQYwKQYDVR0RBCIwIKQeMBwxGjAYBgNVBAMTEVByaXZhdGVMYWJlbDItMTM4MA0GCSqG
SIb3DQEBBQUAA4GBAEiM0VCD6gsuzA2jZqxnD3+vrL7CF6FDlpSdf0whuPg2H6otnzYvwPQc
UCCTcDz9reFhYsPZOhl+hLGZGwDFGguCdJ4lUJRix9sncVcljd2pnDmOjCBPZV+V2vf3h9bG
CE6u9uo05RAaWzVNd+NWIXiC3CEZNd4ksdMdRv9dX2VPMYIDZDCCA2ACAQEwdjBiMQswCQYD
VQQGEwJaQTElMCMGA1UEChMcVGhhd3RlIENvbnN1bHRpbmcgKFB0eSkgTHRkLjEsMCoGA1UE
AxMjVGhhd3RlIFBlcnNvbmFsIEZyZWVtYWlsIElzc3VpbmcgQ0ECEAxljuBYvPVR3WhH6nkb
TPowCQYFKw4DAhoFAKCCAcMwGAYJKoZIhvcNAQkDMQsGCSqGSIb3DQEHATAcBgkqhkiG9w0B
CQUxDxcNMDgwOTIwMTcxNDU0WjAjBgkqhkiG9w0BCQQxFgQUZc6A23TyNkAXEP8Tdp/oXam7
g9kwUgYJKoZIhvcNAQkPMUUwQzAKBggqhkiG9w0DBzAOBggqhkiG9w0DAgICAIAwDQYIKoZI
hvcNAwICAUAwBwYFKw4DAgcwDQYIKoZIhvcNAwICASgwgYUGCSsGAQQBgjcQBDF4MHYwYjEL
MAkGA1UEBhMCWkExJTAjBgNVBAoTHFRoYXd0ZSBDb25zdWx0aW5nIChQdHkpIEx0ZC4xLDAq
BgNVBAMTI1RoYXd0ZSBQZXJzb25hbCBGcmVlbWFpbCBJc3N1aW5nIENBAhAMZY7gWLz1Ud1o
R+p5G0z6MIGHBgsqhkiG9w0BCRACCzF4oHYwYjELMAkGA1UEBhMCWkExJTAjBgNVBAoTHFRo
YXd0ZSBDb25zdWx0aW5nIChQdHkpIEx0ZC4xLDAqBgNVBAMTI1RoYXd0ZSBQZXJzb25hbCBG
cmVlbWFpbCBJc3N1aW5nIENBAhAMZY7gWLz1Ud1oR+p5G0z6MA0GCSqGSIb3DQEBAQUABIIB
AFZVSUu0Mp+jF4mQeTb5Enc6MLdsnOQsCLqFFqTeKnnlPjpF4mpI7MKP5ntUjFwYiNZuBkSD
XPZ8urIFRns2rEvss2NUK+CfhufokwM+upfq1eDyp9vrDsNwG5i8nPQGolaLVoN7j6B2ew4r
sbjz1zB8DZTGlx/GYQ0/CP0UB9NLcCaBz6l8SWMmN7D5THsOUSyFLqDfI4Qo/kjBlTKhCBEH
Wh8+GtFzzVLyBYR5YXNBxhux8FOUONjWjE4Qx/tAJcPuKMBKUOTH9OFWSWaovmuwZGCNbmz1
79NfmBpNnpY4Lil0wySDAG3DItPLkx9xNg0bI+dR1dID09Wn7jnnh5EAAAAAAAA=
--------------ms080407050106020501070902--