You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Nick Kew <ni...@webthing.com> on 2006/11/15 15:24:23 UTC
http://svn.apache.org/viewvc?view=rev&revision=426799
I've been trying to review the mod_deflate updates.
The deflate filter stuff looks good. But I'm confused by the
validation buffer usage in the input filter. You only allocate
a validation buffer at line 1177 (STREAM_END), yet you test
it at line 1134: presumably that'll only ever test positive
on a subsequent call. Shouldn't the block around 1134 be
brought to the top of the function, so it catches the case
of a last brigade containing only an EOS?
--
Nick Kew
Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/
Re: http://svn.apache.org/viewvc?view=rev&revision=426799
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/16/2006 02:25 PM, Plüm wrote:
>
>>-----Ursprüngliche Nachricht-----
>>Von: Nick Kew
>>Gesendet: Donnerstag, 16. November 2006 02:36
>>
>>
>>On Wed, 15 Nov 2006 21:33:07 +0100
>>Ruediger Pluem <rp...@apache.org> wrote:
>>
>>
>>>Because of your question I had to rewalk the code path and I think
>>>I found two other bugs with my code. I fixed them on trunk:
>>>
>>>http://svn.apache.org/viewvc?view=rev&revision=475403
>>
>>Hang on. It's worse than that. Or else I'm suffering
>>"shouldn't be working in the wee hours" syndrome.
>>
>>When you first set up the validation buffer, you copy available
>>data into it, and set validation_buffer_length. Now the memcpy
>>in this section of code is overwriting validation_buffer,
>>when it should be appending to it. Then you increment the
>>buffer_length, and decrement avail_in by the number of bytes
>>appended. At that point, if avail_in is nonzero we might want
>>to log a warning of extra junk.
>
>
> Argh. You are right. Good catch. I have currently no svn access but
Ok. This is what I committed to address the issues found today and yesterday:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_deflate.c?r1=426799&r2=475922
As I know that diffs of diffs become unreadable r426799 + above:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_deflate.c?r1=426795&r2=475922
Provided that you see nothing else I would try to get a minute on the weekend to adjust things
in the STATUS file.
>
>
>>Edge-cases can be notoriously hard to test. I wonder if there's
>>a compression/zlib test suite we could use?
>
>
> Regarding the compressed content I simply used files that I compressed with
> gzip and stripped of the gz extension. I guess the hard case here is to split
Plus
SetOutPutFilter INFLATE
Header set Content-Encoding gzip early
in the configuration as the inflate filter only works on content with content-encoding
set to gzip.
Regards
Rüdiger
Re: http://svn.apache.org/viewvc?view=rev&revision=426799
Posted by Plüm,
Rüdiger,
VF EITO <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Nick Kew
> Gesendet: Donnerstag, 16. November 2006 02:36
>
>
> On Wed, 15 Nov 2006 21:33:07 +0100
> Ruediger Pluem <rp...@apache.org> wrote:
>
> >
> > Because of your question I had to rewalk the code path and I think
> > I found two other bugs with my code. I fixed them on trunk:
> >
> > http://svn.apache.org/viewvc?view=rev&revision=475403
>
> Hang on. It's worse than that. Or else I'm suffering
> "shouldn't be working in the wee hours" syndrome.
>
> When you first set up the validation buffer, you copy available
> data into it, and set validation_buffer_length. Now the memcpy
> in this section of code is overwriting validation_buffer,
> when it should be appending to it. Then you increment the
> buffer_length, and decrement avail_in by the number of bytes
> appended. At that point, if avail_in is nonzero we might want
> to log a warning of extra junk.
Argh. You are right. Good catch. I have currently no svn access but
the patch below should fix this:
Index: mod_deflate.c
===================================================================
--- mod_deflate.c (revision 475613)
+++ mod_deflate.c (working copy)
@@ -1144,20 +1144,24 @@
copy_size = VALIDATION_SIZE - ctx->validation_buffer_length;
if (copy_size > ctx->stream.avail_in)
copy_size = ctx->stream.avail_in;
- memcpy(ctx->validation_buffer, ctx->stream.next_in, copy_size);
- } else {
+ memcpy(ctx->validation_buffer + ctx->validation_buffer_length,
+ ctx->stream.next_in, copy_size);
+ /* Saved copy_size bytes */
+ ctx->stream.avail_in -= copy_size;
+ }
+ if (ctx->stream.avail_in) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Zlib: %d bytes of garbage at the end of "
"compressed stream.", ctx->stream.avail_in);
+ /*
+ * There is nothing worth consuming for zlib left, because it is
+ * either garbage data or the data has been copied to the
+ * validation buffer (processing validation data is no business
+ * for zlib). So set ctx->stream.avail_in to zero to indicate
+ * this to the following while loop.
+ */
+ ctx->stream.avail_in = 0;
}
- /*
- * There is nothing worth consuming for zlib left, because it is
- * either garbage data or the data has been copied to the
- * validation buffer (processing validation data is no business for
- * zlib). So set ctx->stream.avail_in to zero to indicate this to
- * the following while loop.
- */
- ctx->stream.avail_in = 0;
}
zRC = Z_OK;
>
> > http://svn.apache.org/viewvc?view=rev&revision=475406
>
> Why? I'd like to understand what makes that necessary.
I think that I remember cases where two EOS buckets run down the chain. IMHO
this is a bug elsewhere, but I would like to prevent SEGFAULTing in mod_deflate
if this happens. So you might call it defensive programmming here.
>
> Edge-cases can be notoriously hard to test. I wonder if there's
> a compression/zlib test suite we could use?
Regarding the compressed content I simply used files that I compressed with
gzip and stripped of the gz extension. I guess the hard case here is to split
them in a way to buckets and brigades such that all edge cases can be tested.
In this case a filter just after the default handler would be handy that would
allow us to split the brigade from the default handler at certain boundaries and
let us split the buckets inside these brigades at certain predefined boundaries.
Regards
Rüdiger
Re: http://svn.apache.org/viewvc?view=rev&revision=426799
Posted by Nick Kew <ni...@webthing.com>.
On Wed, 15 Nov 2006 21:33:07 +0100
Ruediger Pluem <rp...@apache.org> wrote:
>
> Because of your question I had to rewalk the code path and I think
> I found two other bugs with my code. I fixed them on trunk:
>
> http://svn.apache.org/viewvc?view=rev&revision=475403
Hang on. It's worse than that. Or else I'm suffering
"shouldn't be working in the wee hours" syndrome.
When you first set up the validation buffer, you copy available
data into it, and set validation_buffer_length. Now the memcpy
in this section of code is overwriting validation_buffer,
when it should be appending to it. Then you increment the
buffer_length, and decrement avail_in by the number of bytes
appended. At that point, if avail_in is nonzero we might want
to log a warning of extra junk.
> http://svn.apache.org/viewvc?view=rev&revision=475406
Why? I'd like to understand what makes that necessary.
Edge-cases can be notoriously hard to test. I wonder if there's
a compression/zlib test suite we could use?
--
Nick Kew
Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/
Re: http://svn.apache.org/viewvc?view=rev&revision=426799
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/15/2006 03:24 PM, Nick Kew wrote:
> I've been trying to review the mod_deflate updates.
Thanks for doing so. I really appreciate this.
>
> The deflate filter stuff looks good. But I'm confused by the
> validation buffer usage in the input filter. You only allocate
> a validation buffer at line 1177 (STREAM_END), yet you test
> it at line 1134: presumably that'll only ever test positive
> on a subsequent call. Shouldn't the block around 1134 be
> brought to the top of the function, so it catches the case
> of a last brigade containing only an EOS?
No. If the last brigade does only contain an EOS bucket and the validation buffer
is still incomplete it does mean that the validation data for the whole stream
is incomplete.
If we hit an EOS bucket all validation data must be available.
Keep in mind that we process validation data for ourselves. We
do not have to push this data through the inflate function and
have it inflate to the ctx->buffer. We read it directly from the bucket.
I hope this make things more clear. If not, please do not hesitate to ask me
until I see an error or explained it clearly :-).
Because of your question I had to rewalk the code path and I think
I found two other bugs with my code. I fixed them on trunk:
http://svn.apache.org/viewvc?view=rev&revision=475403
http://svn.apache.org/viewvc?view=rev&revision=475406
It would be nice if you could give me a short feedback on them.
If you agree with them I would incorporate them in the backport
proposal.
Regards
Rüdiger