Re: [PATCH] mod_deflate + mod_proxy bug
On Wed, Jun 09, 2004 at 05:23:38PM -0400, Allan Edwards wrote: Running ProxyPass with mod_deflate results in an extraneous 20 bytes being tacked onto 304 responses from the backend. The problem is that mod_deflate doesn't handle the zero byte body, adds the gzip header and tries to compress 0 bytes. This patch detects the fact that there was no data to compress and removes the gzip header from the bucket brigade. Wouldn't it be simpler to just check for a brigade containing just EOS and do nothing for that case in the first place? But the fact that the proxy passes such a brigade through the output filters in the first place sounds like the real bug, it doesn't happen for a non-proxied 304 response. joe
Re: [PATCH] mod_deflate + mod_proxy bug
On Wed, 9 Jun 2004, Allan Edwards wrote: Running ProxyPass with mod_deflate results in an extraneous 20 bytes being tacked onto 304 responses from the backend. The problem is that mod_deflate doesn't handle the zero byte body, adds the gzip header and tries to compress 0 bytes. This patch detects the fact that there was no data to compress and removes the gzip header from the bucket brigade. Any comments before I commit to head? This is part of a slightly broader problem with proxying and mod_deflate: it'll also waste time gzipping already-compressed data from the backend in those cases where the compression is not explicitly indicated in the Content-Encoding header. Obvious examples are all the main image formats. I'm currently running a hack that works around this, and planning a better review when time permits (i.e. when I've caught up with things after http://www.theatreroyal.com/showpage.php?dd=1theid=2578 which now has three nights left to run). More interesting is the entire subject of filtering in a dynamic context such as a proxy. The directives available to control filtering are simply not up to it. Watch this space:-) -- Nick Kew
Re: [PATCH] mod_deflate + mod_proxy bug
Joe Orton wrote: Wouldn't it be simpler to just check for a brigade containing just EOS and do nothing for that case in the first place? Yes I had considered that. The initial patch covered some pathological cases but after having looked over the code some more I think the simpler more efficient way of bailing on just EOS is sufficient. But the fact that the proxy passes such a brigade through the output filters in the first place sounds like the real bug, it doesn't happen for a non-proxied 304 response. Whether or not this is a bug I guess is open for debate. What happens for non proxied error responses is that ap_send_error_response resets r-output_flters to r-proto_output_filters so the deflate filter is taken out of the respone path. In the proxy path there is no such logic for error responses, so error pages would get zipped. But I don't know that this violates the RFC and browsers seem to be able to handle it. Allan
[PATCH] mod_deflate + mod_proxy bug
Running ProxyPass with mod_deflate results in
an extraneous 20 bytes being tacked onto 304
responses from the backend.
The problem is that mod_deflate doesn't handle
the zero byte body, adds the gzip header and
tries to compress 0 bytes.
This patch detects the fact that there was no
data to compress and removes the gzip header
from the bucket brigade.
Any comments before I commit to head?
Allan
--
Index: mod_deflate.c
===
RCS file: /home/cvs/httpd-2.0/modules/filters/mod_deflate.c,v
retrieving revision 1.49
diff -u -d -b -r1.49 mod_deflate.c
--- mod_deflate.c 1 Jun 2004 13:06:10 - 1.49
+++ mod_deflate.c 9 Jun 2004 16:38:30 -
@@ -433,6 +433,8 @@
char *buf;
unsigned int deflate_len;
+if (ctx-stream.total_in != 0) {
+
ctx-stream.avail_in = 0; /* should be zero already anyway */
for (;;) {
deflate_len = c-bufferSize - ctx-stream.avail_out;
@@ -510,6 +512,14 @@
* Time to pass it along down the chain.
*/
return ap_pass_brigade(f-next, ctx-bb);
+}
+else {
+/* this was a zero length response, remove gzip header bucket then
pass down the EOS */
+APR_BUCKET_REMOVE(APR_BRIGADE_FIRST(ctx-bb));
+APR_BUCKET_REMOVE(e);
+APR_BRIGADE_INSERT_TAIL(ctx-bb, e);
+return ap_pass_brigade(f-next, ctx-bb);
+}
}
if (APR_BUCKET_IS_FLUSH(e)) {
Re: [PATCH] mod_deflate + mod_proxy bug
On Wed, 9 Jun 2004, Allan Edwards wrote:
+}
+else {
+/* this was a zero length response, remove gzip header bucket then
pass down the EOS */
+APR_BUCKET_REMOVE(APR_BRIGADE_FIRST(ctx-bb));
+APR_BUCKET_REMOVE(e);
+APR_BRIGADE_INSERT_TAIL(ctx-bb, e);
+return ap_pass_brigade(f-next, ctx-bb);
+}
I haven't looked at the entire context of this, but if you remove a bucket
(brigade_first(ctx-bb) from a brigade without deleting it and without
having any extra pointers to it, you'll leak memory.
Also, what happens if e *is* the first bucket in the brigade? Can that
occur? I think that by coincidence given the implementation of
APR_BUCKET_REMOVE, nothing bad would happen by double-removing a given
bucket twice in a row, but in general that seems like a bad idea and
should be avoided.
--Cliff
Re: [PATCH] mod_deflate + mod_proxy bug
Cliff Woolley wrote: I haven't looked at the entire context of this, but if you remove a bucket (brigade_first(ctx-bb) from a brigade without deleting it and without having any extra pointers to it, you'll leak memory. Thanks for catching that! I'll replace APR_BUCKET_REMOVE with a call to apr_bucket_delete(). Also just realized I need to add a call to deflateEnd(). Also, what happens if e *is* the first bucket in the brigade? Can that occur? I think that by coincidence given the implementation of APR_BUCKET_REMOVE, nothing bad would happen by double-removing a given bucket twice in a row, but in general that seems like a bad idea and should be avoided. e is on the brigade passed into deflate_out_filter and the gzip header bucket is in ctx-bb so that is not a problem. Thanks, Allan
Re: [PATCH] mod_deflate + mod_proxy bug
On Wed, 9 Jun 2004, Allan Edwards wrote: Also just realized I need to add a call to deflateEnd(). Oh right, that too. :-) e is on the brigade passed into deflate_out_filter and the gzip header bucket is in ctx-bb so that is not a problem. Ah, yeah, that would make sense. Cool.
