[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/

Yann Ylavic <> writes:

>> I am thinking about fixing this with the attached patch and proposing it for
>> backport to 2.4.x.
>> Would there be any objections to that?
> +1 for the patch, I missed the separate 304 handling in
> mod_brotli/deflate, thanks for catching this!

Thanks, I will commit it at the earliest opportunity.

> It seems that the 304 "shortcut" happens too late though, after
> entity/content-* headers are added to the response, which does not
> look right. Don't we need something like the attached patch too?

I haven't checked it in details, but at first glance I think that this patch
could break a few cases.

One example would be a case with several filters working in a chain for a
304 response.  The first of them sees Content-Encoding identity, performs
some fix-ups, such as the one in deflate_check_etag() and removes itself
from the chain without altering the r->content-encoding or the Content-
Encoding header value.  The next filter then sees the C-E identity again,
decides to do another fix-up before bailing out, and thus results in an
incorrect ETag value or something similar.

 (An interesting observation is that does an
  opposite of this patch by ensuring that C-E is actually set prior to bailing
  out and removing itself when dealing with 304's.)

However, a more important question is whether there is an actual problem to
solve.  I see that ap_http_header_filter() features a whitelist of headers
that are sent for 304 responses (http_filters.c:1428), and all headers such
as Content-Encoding are filtered anyway.

So maybe the current state doesn't require fixing at all — assuming that
neither mod_deflate nor mod_brotli can actually begin streaming the 304
response with the (unexpected) set of headers — which I don't think could
be happening.

Evgeny Kotkov