2018-04-09 22:38 GMT+02:00 Luca Toscano <toscano.l...@gmail.com>:
> Hi everybody,
>
> 2018-04-05 7:59 GMT+02:00 <bugzi...@apache.org>:
>
>> https://bz.apache.org/bugzilla/show_bug.cgi?id=61860
>>
>> --- Comment #4 from Luca Toscano <toscano.l...@gmail.com> ---
>> Ok now I think I know what's happening (and I got what Eric was trying to
>> suggest). One of the things that ap_send_error_response does is running
>> ap_run_insert_error_filter, that allows modules to insert their filters
>> (that
>> will be executed). mod_headers does it, specifically it adds this bit:
>>
>> /*
>> * Make sure we propagate any "Header always" settings on the error
>> * path through http_protocol.c.
>> */
>> static apr_status_t ap_headers_error_filter(ap_filter_t *f,
>> apr_bucket_brigade *in)
>>
>> It makes sure that the Headers set via 'always' are in err_headers_out, to
>> allow them to be added in the response. The issue in my opinion is that in
>> ap_send_error_respose we swap r->headers_out with r->err_headers_out, and
>> clear
>> r->err_headers_out (that will be re-populated afterwards). Should we
>> simply do:
>>
>> Index: modules/http/http_protocol.c
>> ===================================================================
>> --- modules/http/http_protocol.c (revision 1828234)
>> +++ modules/http/http_protocol.c (working copy)
>> @@ -1262,7 +1262,6 @@
>> }
>>
>> if (!r->assbackwards) {
>> - apr_table_t *tmp = r->headers_out;
>>
>> /* For all HTTP/1.x responses for which we generate the message,
>> * we need to avoid inheriting the "normal status" header fields
>> @@ -1269,9 +1268,7 @@
>> * that may have been set by the request handler before the
>> * error or redirect, except for Location on external redirects.
>> */
>> - r->headers_out = r->err_headers_out;
>> - r->err_headers_out = tmp;
>> - apr_table_clear(r->err_headers_out);
>> + apr_table_clear(r->headers_out);
>>
>> if (ap_is_HTTP_REDIRECT(status) || (status == HTTP_CREATED)) {
>> if ((location != NULL) && *location) {
>>
>>
> I am testing the above patch as possible solution for
> https://bz.apache.org/bugzilla/show_bug.cgi?id=61860, in which a user
> reports that a range error request gets headers duplicated (more
> specifically, all the ones set via Header always). Is there anything big
> that I am missing? I think that it these situations httpd should just use
> r->err_headers_out..
>
Still working on this, for the moment I haven't found any clear regression
when applying the following:
Index: modules/http/http_protocol.c
===================================================================
--- modules/http/http_protocol.c (revision 1828234)
+++ modules/http/http_protocol.c (working copy)
@@ -1262,7 +1262,6 @@
}
if (!r->assbackwards) {
- apr_table_t *tmp = r->headers_out;
/* For all HTTP/1.x responses for which we generate the message,
* we need to avoid inheriting the "normal status" header fields
@@ -1269,9 +1268,7 @@
* that may have been set by the request handler before the
* error or redirect, except for Location on external redirects.
*/
- r->headers_out = r->err_headers_out;
- r->err_headers_out = tmp;
- apr_table_clear(r->err_headers_out);
+ apr_table_clear(r->headers_out);
if (ap_is_HTTP_REDIRECT(status) || (status == HTTP_CREATED)) {
if ((location != NULL) && *location) {
The above bit seems to solve the issue reported by the user in PR 61860.
This code has been working for a long time so I am wondering if I am
missing any use cases, but IIUC swapping headers_out with err_headers_out
seems to deviate from what's written in httpd.h:
* The difference between headers_out and err_headers_out is that the
* latter are printed even on error, and persist across internal
redirects
* (so the headers printed for ErrorDocument handlers will have them).
Any suggestion is really appreciated :)
Thanks!
Luca
