Re: stop copying footers to r-headers_in?
Candidate patch uses %I and %O but they are used by mod_logio. It is
hard to find two good unused characters.
What do people think about allowing two-character log formats? I
think patch below only breaks someone who had a %XX where XX is a
registered two digit tag and they expect the 1 char + literal (seems
safe enough to me even for 2.2)
http://people.apache.org/~covener/patches/httpd-trunk-logger-2chars.diff
Patch includes the trailers stuff in mod_log_config.
Test seems to work, normal access log unchanged and %{foo}Ti hits with
printf GET / HTTP/1.1\r\nHost: localhost\r\nTransfer-Encoding:
chunked\r\n\r\n0\r\nFoo: bar\r\n\r\n |nc 0 80
On Wed, Jun 25, 2014 at 1:23 PM, Edward Lu chaos...@gmail.com wrote:
Patch for trunk as well
On Wed, Jun 25, 2014 at 1:20 PM, Edward Lu chaos...@gmail.com wrote:
Wanted to follow up on this thread again; here's my latest patch on 2.2.x
that takes some ideas from Joe's patch. I also merge the trailers into the
headers after reading them now, instead of directly appending them into the
headers.
I will leave the PROXYREQ_RESPONSE case to someone else, as I still don't
fully understand it.
On Thu, May 15, 2014 at 3:20 PM, Yann Ylavic ylavic@gmail.com wrote:
I did not realize your mail is 8 days old, I just received it -- gmail
is having fun these days :(
Sorry for this misunderstanding and the reply that goes along...
Regards,
Yann.
--
Eric Covener
cove...@gmail.com
Re: stop copying footers to r-headers_in?
something odd in proxy path when backend has
#0 0x7f16f51b92e1 in apr_table_clear (t=0x0) at tables/apr_tables.c:467
467 t-a.nelts = 0;
(gdb) where
#0 0x7f16f51b92e1 in apr_table_clear (t=0x0) at tables/apr_tables.c:467
#1 0x00483bfc in read_chunked_trailers (ctx=0x7f16ac0125c0,
f=0x7f16ac012378, b=0x7f16ac00eec8, merge=0) at http_filters.c:198
#2 0x0048493d in ap_http_filter (f=0x7f16ac012378,
b=0x7f16ac00eec8, mode=AP_MODE_READBYTES, block=APR_NONBLOCK_READ,
readbytes=8192) at http_filters.c:448
#3 0x00439d8e in ap_get_brigade (next=0x7f16ac012378,
bb=0x7f16ac00eec8, mode=AP_MODE_READBYTES, block=APR_NONBLOCK_READ,
readbytes=8192) at util_filter.c:553
#4 0x7f16edb39794 in ap_proxy_http_process_response
(p=0x7f16ac004908, r=0x7f16ac004980, backend_ptr=0x7f16ea0c1a30,
worker=0x10154c8, conf=0x1015238,
server_portstr=0x7f16ea0c1a70 ) at mod_proxy_http.c:1693
#5 0x7f16edb3 in proxy_http_handler (r=0x7f16ac004980,
worker=0x10154c8, conf=0x1015238, url=0x7f16ac0063e6
http://127.0.0.1:81/trailer.asis;, proxyname=0x0, proxyport=0)
at mod_proxy_http.c:2049
#6 0x7f16edf51858 in proxy_run_scheme_handler (r=0x7f16ac004980,
worker=0x10154c8, conf=0x1015238, url=0x7f16ac0063e6
http://127.0.0.1:81/trailer.asis;, proxyhost=0x0,
proxyport=0) at mod_proxy.c:2798
#7 0x7f16edf4c650 in proxy_handler (r=0x7f16ac004980) at mod_proxy.c:1157
#8 0x00463ad1 in ap_run_handler (r=0x7f16ac004980) at config.c:169
#9 0x00464524 in ap_invoke_handler (r=0x7f16ac004980) at config.c:433
#10 0x004827ba in ap_process_async_request (r=0x7f16ac004980)
at http_request.c:317
#11 0x0047ebbb in ap_process_http_async_connection
(c=0x7f16e40b3790) at http_core.c:143
#12 0x0047eda8 in ap_process_http_connection
(c=0x7f16e40b3790) at http_core.c:228
#13 0x00471fc9 in ap_run_process_connection (c=0x7f16e40b3790)
at connection.c:41
#14 0x7f16ecea8f2c in process_socket (thd=0x111da28,
p=0x7f16e40b3488, sock=0x7f16e40b3500, cs=0x7f16e40b3708,
my_child_num=0, my_thread_num=0) at event.c:1077
#15 0x7f16eceabd98 in worker_thread (thd=0x111da28,
dummy=0x7f16e40008c0) at event.c:2141
#16 0x7f16f51d1f2a in dummy_worker (opaque=0x111da28) at
threadproc/unix/thread.c:142
#17 0x7f16f4f8c182 in start_thread (arg=0x7f16ea0c2700) at
pthread_create.c:312
(gdb) print *r
$1 = {pool = 0x7f16ac010a48, connection = 0x7f16ac008bb0, server =
0xfe2d08, next = 0x0, prev = 0x0, main = 0x0, the_request = 0x0,
assbackwards = 0, proxyreq = 3, header_only = 0,
proto_num = 0, protocol = 0x0, hostname = 0x0, request_time =
1405432560125880, status_line = 0x0, status = 200, method_number = 0,
method = 0x0, allowed = 0,
allowed_xmethods = 0x0, allowed_methods = 0x0, sent_bodyct = 0,
bytes_sent = 0, mtime = 0, range = 0x0, clength = 0, chunked = 0,
read_body = 0, read_chunked = 0,
expecting_100 = 0, kept_body = 0x0, body_table = 0x0, remaining = 0,
read_length = 0, headers_in = 0x0, headers_out = 0x7f16ac011938,
err_headers_out = 0x7f16ac011d20,
subprocess_env = 0x7f16ac011360, notes = 0x7f16ac011ec0,
content_type = 0x0, handler = 0x0, content_encoding = 0x0,
content_languages = 0x0, vlist_validator = 0x0, user = 0x0,
ap_auth_type = 0x0, unparsed_uri = 0x0, uri = 0x0, filename = 0x0,
canonical_filename = 0x0, path_info = 0x0, args = 0x0, used_path_info
= 0, eos_sent = 0, per_dir_config = 0x0,
request_config = 0x7f16ac012060, log = 0xfe2d28, log_id = 0x0,
htaccess = 0x0, output_filters = 0x7f16ac00cea8, input_filters =
0x7f16ac012378,
proto_output_filters = 0x7f16ac00cea8, proto_input_filters =
0x7f16ac012378, no_cache = 0, no_local_copy = 0, invoke_mtx = 0x0,
parsed_uri = {scheme = 0x0, hostinfo = 0x0,
user = 0x0, password = 0x0, hostname = 0x0, port_str = 0x0, path =
0x0, query = 0x0, fragment = 0x0, hostent = 0x0, port = 0,
is_initialized = 0, dns_looked_up = 0,
dns_resolved = 0}, finfo = {pool = 0x0, valid = 0, protection = 0,
filetype = APR_NOFILE, user = 0, group = 0, inode = 0, device = 0,
nlink = 0, size = 0, csize = 0, atime = 0,
mtime = 0, ctime = 0, fname = 0x0, name = 0x0, filehand = 0x0},
useragent_addr = 0x7f16ac008ab0, useragent_ip = 0x7f16ac008ca0
127.0.0.1, trailers_in = 0x0,
trailers_out = 0x7f16ac011b80}
I use mod_asis + proxy
Status: 200 OK
Content-Type: text/plain
Transfer-Encoding: chunked
1
a
0
Foo: quux
On Wed, Jun 25, 2014 at 1:23 PM, Edward Lu chaos...@gmail.com wrote:
Patch for trunk as well
On Wed, Jun 25, 2014 at 1:20 PM, Edward Lu chaos...@gmail.com wrote:
Wanted to follow up on this thread again; here's my latest patch on 2.2.x
that takes some ideas from Joe's patch. I also merge the trailers into the
headers after reading them now, instead of directly appending them into the
headers.
I will leave the PROXYREQ_RESPONSE case to someone else, as I still don't
fully understand it.
On Thu, May 15, 2014 at 3:20 PM, Yann Ylavic ylavic@gmail.com wrote:
RE: stop copying footers to r-headers_in?
Can you try if the following patch on top fixes this: Index: mod_proxy_http.c === --- mod_proxy_http.c(revision 1610607) +++ mod_proxy_http.c(working copy) @@ -1003,9 +1003,11 @@ rp-status = HTTP_OK; rp-headers_in = apr_table_make(pool, 50); +rp-trailers_in = apr_table_make(pool, 5); rp-subprocess_env = apr_table_make(pool, 50); rp-headers_out = apr_table_make(pool, 12); rp-err_headers_out = apr_table_make(pool, 5); +rp-trailers_out= apr_table_make(pool, 5); rp-notes = apr_table_make(pool, 5); rp-server = r-server; Regards Rüdiger -Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 16:02 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? something odd in proxy path when backend has
Re: stop copying footers to r-headers_in?
Yes, thanks! The patch I started with had out but not in initialized there On Tue, Jul 15, 2014 at 10:20 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: Can you try if the following patch on top fixes this: Index: mod_proxy_http.c === --- mod_proxy_http.c(revision 1610607) +++ mod_proxy_http.c(working copy) @@ -1003,9 +1003,11 @@ rp-status = HTTP_OK; rp-headers_in = apr_table_make(pool, 50); +rp-trailers_in = apr_table_make(pool, 5); rp-subprocess_env = apr_table_make(pool, 50); rp-headers_out = apr_table_make(pool, 12); rp-err_headers_out = apr_table_make(pool, 5); +rp-trailers_out= apr_table_make(pool, 5); rp-notes = apr_table_make(pool, 5); rp-server = r-server; Regards Rüdiger -Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 16:02 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? something odd in proxy path when backend has -- Eric Covener cove...@gmail.com
RE: stop copying footers to r-headers_in?
-Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 15:25 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? Candidate patch uses %I and %O but they are used by mod_logio. It is hard to find two good unused characters. What do people think about allowing two-character log formats? I think patch below only breaks someone who had a %XX where XX is a registered two digit tag and they expect the 1 char + literal (seems safe enough to me even for 2.2) Is there a way for people with such a setup to fix this with a different config? From the top of my head I would say no and that would be a blocker. Regards Rüdiger
Re: stop copying footers to r-headers_in?
On Jul 15, 2014, at 10:38 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: -Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 15:25 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? Candidate patch uses %I and %O but they are used by mod_logio. It is hard to find two good unused characters. What do people think about allowing two-character log formats? I think patch below only breaks someone who had a %XX where XX is a registered two digit tag and they expect the 1 char + literal (seems safe enough to me even for 2.2) Is there a way for people with such a setup to fix this with a different config? From the top of my head I would say no and that would be a blocker. Agreed
Re: stop copying footers to r-headers_in?
On Tue, Jul 15, 2014 at 10:38 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: Is there a way for people with such a setup to fix this with a different config? From the top of my head I would say no and that would be a blocker. Do you think the required '^' prefix in the followup is enough? In this case, to be broken, you would have a custom module that accepted %^ AND you would have had two constant characters in your format that match a newly added format. -- Eric Covener cove...@gmail.com
RE: stop copying footers to r-headers_in?
-Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 16:52 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? On Tue, Jul 15, 2014 at 10:38 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: Is there a way for people with such a setup to fix this with a different config? From the top of my head I would say no and that would be a blocker. Do you think the required '^' prefix in the followup is enough? In I think I missed the follow up. Where can I find it? Regards Rüdiger
Re: stop copying footers to r-headers_in?
On Tue, Jul 15, 2014 at 10:56 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: -Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 16:52 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? On Tue, Jul 15, 2014 at 10:38 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: Is there a way for people with such a setup to fix this with a different config? From the top of my head I would say no and that would be a blocker. Do you think the required '^' prefix in the followup is enough? In I think I missed the follow up. Where can I find it? http://svn.apache.org/viewvc?view=revisionrevision=r1610707
RE: stop copying footers to r-headers_in?
-Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 17:02 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? On Tue, Jul 15, 2014 at 10:56 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: -Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 16:52 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? On Tue, Jul 15, 2014 at 10:38 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: Is there a way for people with such a setup to fix this with a different config? From the top of my head I would say no and that would be a blocker. Do you think the required '^' prefix in the followup is enough? In I think I missed the follow up. Where can I find it? http://svn.apache.org/viewvc?view=revisionrevision=r1610707 This should work. But don't you need to register then ^Ti instead of Ti? Regards Rüdiger
Re: stop copying footers to r-headers_in?
On Tue, Jul 15, 2014 at 11:07 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: This should work. But don't you need to register then ^Ti instead of Ti? yeah, it needs to be registered and used in httpd.conf w/ the ^ -- Eric Covener cove...@gmail.com
RE: stop copying footers to r-headers_in?
That would be at least half my fault for responding off-list... Here it is: Thanks -- I went with ^ Shouldn't have much worry about responding to dev@ On Tue, Jul 15, 2014 at 9:42 AM, Houser, Rick rick.hou...@us.pgds.com wrote: New to this list, so responding directly until I have a better feel Wouldn't a three character tag be safer with the first being a dedicated (unused) prefix to unlock the additional bytes, like Unicode? Something like this: _char1char2 (note, I didn't check if _ was used -- just using it as an example) Rick Houser Rick Houser Web Administration (517)367-3516 -Original Message- From: Plüm, Rüdiger, Vodafone Group [mailto:ruediger.pl...@vodafone.com] Sent: Tuesday, July 15, 2014 10:56 AM To: dev@httpd.apache.org Subject: RE: stop copying footers to r-headers_in? -Original Message- From: Eric Covener [mailto:cove...@gmail.com] Sent: Dienstag, 15. Juli 2014 16:52 To: Apache HTTP Server Development List Subject: Re: stop copying footers to r-headers_in? On Tue, Jul 15, 2014 at 10:38 AM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: Is there a way for people with such a setup to fix this with a different config? From the top of my head I would say no and that would be a blocker. Do you think the required '^' prefix in the followup is enough? In I think I missed the follow up. Where can I find it? Regards Rüdiger
Re: stop copying footers to r-headers_in?
On 15 Jul 2014, at 15:38, Rüdiger Plüm ruediger.pl...@vodafone.com wrote:
-Original Message-
From: Eric Covener [mailto:cove...@gmail.com]
Sent: Dienstag, 15. Juli 2014 15:25
To: Apache HTTP Server Development List
Subject: Re: stop copying footers to r-headers_in?
What do people think about allowing two-character log formats? I
think patch below only breaks someone who had a %XX where XX is a
registered two digit tag and they expect the 1 char + literal (seems
safe enough to me even for 2.2)
Is there a way for people with such a setup to fix this with a different
config?
From the top of my head I would say no and that would be a blocker.
%{thing}[label] perhaps? Maybe it's too confusing / contrived, maybe it's not
needed (yet).
Like this, for an imaginary new format string label %[foo]
LogFormat %{%Y-%m-%d}tT%{%H:%M:%S}t%{usec_frac}t %{usec_frac}[foo]
and for another label %[bar]:
LogFormat %{sec}t%{msec_frac}t %s %[bar] %L %{REQUEST_STATUS} -strcmatch
'5*'
--
Tim Bannister – is...@jellybaby.net
Re: stop copying footers to r-headers_in?
Wanted to follow up on this thread again; here's my latest patch on 2.2.x
that takes some ideas from Joe's patch. I also merge the trailers into the
headers after reading them now, instead of directly appending them into the
headers.
I will leave the PROXYREQ_RESPONSE case to someone else, as I still don't
fully understand it.
On Thu, May 15, 2014 at 3:20 PM, Yann Ylavic ylavic@gmail.com wrote:
I did not realize your mail is 8 days old, I just received it -- gmail
is having fun these days :(
Sorry for this misunderstanding and the reply that goes along...
Regards,
Yann.
diff --git a/include/http_core.h b/include/http_core.h
index 7be9d5d..1aa9bd9 100644
--- a/include/http_core.h
+++ b/include/http_core.h
@@ -613,6 +613,10 @@ typedef struct {
#define AP_TRACE_ENABLE1
#define AP_TRACE_EXTENDED 2
int trace_enable;
+#define AP_MERGE_TRAILERS_UNSET0
+#define AP_MERGE_TRAILERS_ENABLE 1
+#define AP_MERGE_TRAILERS_DISABLE 2
+int merge_trailers;
} core_server_config;
diff --git a/include/httpd.h b/include/httpd.h
index f353d9f..5264184 100644
--- a/include/httpd.h
+++ b/include/httpd.h
@@ -1006,6 +1006,11 @@ struct request_rec {
* record to improve 64bit alignment the next time we need to break
* binary compatibility for some other reason.
*/
+
+/** MIME trailer environment from the request */
+apr_table_t *trailers_in;
+/** MIME trailer environment from the response */
+apr_table_t *trailers_out;
};
/**
diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index 8b6e371..480aba7 100644
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -206,6 +206,50 @@ static apr_status_t get_chunk_line(http_ctx_t *ctx, apr_bucket_brigade *b,
}
+static apr_status_t read_chunked_trailers(http_ctx_t *ctx, ap_filter_t *f,
+ apr_bucket_brigade *b, int merge)
+{
+int rv;
+apr_bucket *e;
+request_rec *r = f-r;
+apr_table_t *saved_headers_in = r-headers_in;
+int saved_status = r-status;
+
+r-status = HTTP_OK;
+r-headers_in = r-trailers_in;
+apr_table_clear(r-headers_in);
+
+ctx-state = BODY_NONE;
+ap_get_mime_headers(r);
+
+if(r-status == HTTP_OK) {
+r-status = saved_status;
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(b, e);
+ctx-eos_sent = 1;
+rv = APR_SUCCESS;
+}
+else {
+const char *error_notes = apr_table_get(r-notes,
+error-notes);
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ Error while reading HTTP trailer: %i%s%s,
+ r-status, error_notes ? : : ,
+ error_notes ? error_notes : );
+rv = APR_EINVAL;
+}
+
+if(!merge) {
+r-headers_in = saved_headers_in;
+}
+else {
+r-headers_in = apr_table_overlay(r-pool, saved_headers_in,
+r-trailers_in);
+}
+
+return rv;
+}
+
/* This is the HTTP_INPUT filter for HTTP requests and responses from
* proxied servers (mod_proxy). It handles chunked and content-length
* bodies. This can only be inserted/used after the headers
@@ -215,6 +259,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
ap_input_mode_t mode, apr_read_type_e block,
apr_off_t readbytes)
{
+core_server_config *conf;
apr_bucket *e;
http_ctx_t *ctx = f-ctx;
apr_status_t rv;
@@ -222,6 +267,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
int http_error = HTTP_REQUEST_ENTITY_TOO_LARGE;
apr_bucket_brigade *bb;
+conf = (core_server_config *)
+ap_get_module_config(f-r-server-module_config, core_module);
+
+
/* just get out of the way of things we don't want. */
if (mode != AP_MODE_READBYTES mode != AP_MODE_GETLINE) {
return ap_get_brigade(f-next, b, mode, block, readbytes);
@@ -404,12 +453,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
if (!ctx-remaining) {
/* Handle trailers by calling ap_get_mime_headers again! */
-ctx-state = BODY_NONE;
-ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+return read_chunked_trailers(ctx, f, b,
+conf-merge_trailers == AP_MERGE_TRAILERS_ENABLE);
}
}
}
@@ -510,12 +555,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
if (!ctx-remaining) {
/* Handle trailers by calling ap_get_mime_headers again! */
-ctx-state = BODY_NONE;
-ap_get_mime_headers(f-r);
-e =
Re: stop copying footers to r-headers_in?
Patch for trunk as well
On Wed, Jun 25, 2014 at 1:20 PM, Edward Lu chaos...@gmail.com wrote:
Wanted to follow up on this thread again; here's my latest patch on 2.2.x
that takes some ideas from Joe's patch. I also merge the trailers into the
headers after reading them now, instead of directly appending them into the
headers.
I will leave the PROXYREQ_RESPONSE case to someone else, as I still don't
fully understand it.
On Thu, May 15, 2014 at 3:20 PM, Yann Ylavic ylavic@gmail.com wrote:
I did not realize your mail is 8 days old, I just received it -- gmail
is having fun these days :(
Sorry for this misunderstanding and the reply that goes along...
Regards,
Yann.
diff --git a/include/http_core.h b/include/http_core.h
index 337859b..3ed3793 100644
--- a/include/http_core.h
+++ b/include/http_core.h
@@ -669,6 +669,10 @@ typedef struct {
#define AP_TRACE_ENABLE1
#define AP_TRACE_EXTENDED 2
int trace_enable;
+#define AP_MERGE_TRAILERS_UNSET0
+#define AP_MERGE_TRAILERS_ENABLE 1
+#define AP_MERGE_TRAILERS_DISABLE 2
+int merge_trailers;
apr_array_header_t *conn_log_level;
diff --git a/include/httpd.h b/include/httpd.h
index d458ad7..f3b07f3 100644
--- a/include/httpd.h
+++ b/include/httpd.h
@@ -1039,6 +1039,11 @@ struct request_rec {
*/
apr_sockaddr_t *useragent_addr;
char *useragent_ip;
+
+/** MIME trailer environment from the request */
+apr_table_t *trailers_in;
+/** MIME trailer environment from the response */
+apr_table_t *trailers_out;
};
/**
diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index 11d5cf8..6148c1b 100644
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -184,6 +184,48 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
return APR_SUCCESS;
}
+static apr_status_t read_chunked_trailers(http_ctx_t *ctx, ap_filter_t *f,
+ apr_bucket_brigade *b, int merge)
+{
+int rv;
+apr_bucket *e;
+request_rec *r = f-r;
+apr_table_t *saved_headers_in = r-headers_in;
+int saved_status = r-status;
+
+r-status = HTTP_OK;
+r-headers_in = r-trailers_in;
+apr_table_clear(r-headers_in);
+ap_get_mime_headers(r);
+
+if(r-status == HTTP_OK) {
+r-status = saved_status;
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(b, e);
+ctx-eos_sent = 1;
+rv = APR_SUCCESS;
+}
+else {
+const char *error_notes = apr_table_get(r-notes,
+error-notes);
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ Error while reading HTTP trailer: %i%s%s,
+ r-status, error_notes ? : : ,
+ error_notes ? error_notes : );
+rv = APR_EINVAL;
+}
+
+if(!merge) {
+r-headers_in = saved_headers_in;
+}
+else {
+r-headers_in = apr_table_overlay(r-pool, saved_headers_in,
+r-trailers_in);
+}
+
+return rv;
+}
+
/* This is the HTTP_INPUT filter for HTTP requests and responses from
* proxied servers (mod_proxy). It handles chunked and content-length
* bodies. This can only be inserted/used after the headers
@@ -192,12 +234,16 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
{
+core_server_config *conf;
apr_bucket *e;
http_ctx_t *ctx = f-ctx;
apr_status_t rv;
apr_off_t totalread;
int again;
+conf = (core_server_config *)
+ap_get_module_config(f-r-server-module_config, core_module);
+
/* just get out of the way of things we don't want. */
if (mode != AP_MODE_READBYTES mode != AP_MODE_GETLINE) {
return ap_get_brigade(f-next, b, mode, block, readbytes);
@@ -396,11 +442,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
again = 1; /* come around again */
if (ctx-state == BODY_CHUNK_TRAILER) {
-ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+/* Treat UNSET as DISABLE - trailers aren't merged by default */
+int merge_trailers =
+conf-merge_trailers == AP_MERGE_TRAILERS_ENABLE;
+return read_chunked_trailers(ctx, f, b, merge_trailers);
}
break;
diff --git a/modules/http/http_request.c b/modules/http/http_request.c
index 796d506..cdfec8b 100644
--- a/modules/http/http_request.c
+++ b/modules/http/http_request.c
@@ -463,6 +463,7 @@ static request_rec *internal_internal_redirect(const char *new_uri,
new-main
Re: stop copying footers to r-headers_in?
Added mod_log_config changes; I used O and I for the format letters.
Not sure if there are letters that might be more preferred.
On Fri, May 9, 2014 at 11:01 AM, Edward Lu chaos...@gmail.com wrote:
Here's a 2.2.x backport of Yann's patch with a directive, MergeTrailers,
added in to opt-in to the old behavior. By default, it doesn't merge the
trailers. I'm working on adding the capability to mod_log_config to log the
trailers, but I figured I'd post this patch first to get some review.
- Ed
On Tue, May 6, 2014 at 7:45 PM, Eric Covener cove...@gmail.com wrote:
On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic ylavic@gmail.com wrote:
On Tue, May 6, 2014 at 1:41 PM, Eric Covener cove...@gmail.com wrote:
I'd really like to start chucking trailers, but the scale of these
patches really frightens me for 2.4 and 2.2 and accompanying what will
likely be a security roll-up.
Understood.
Is anyone sitting on a more tactical version of the fix?
Well, maybe I should stop proposing something here but maybe the
attached patch can help.
I think it's a tactical version of the previous one, in that it does
*not* touch ap_get_mime_headers*() (nor ap_rgetline*()), but
saves/restores the data (request's fields, notes) modified by the
function when called from ap_http_filter().
It could be a way for 2.2.x, but I don't think it's enough for trunk
or even 2.4.x, because this implementation still breaks non-blocking
mode (hence event), which is (also) why the core functions were
modified in the previous patch.
I think I could be on board for that plan. Any others?
This patch (still) does not propose to merge splitted trailers into
headers (for those broken by the change), but when (and why) would we
do that?
For 2.2, I would personally want to have the ability to restore the
old merging behavior before any release. Joe drew his line there as
well.
But I think Ed Lu is lurking and can take that last bit on.
--
Eric Covener
cove...@gmail.com
Index: include/http_core.h
===
--- include/http_core.h (revision 1593540)
+++ include/http_core.h (working copy)
@@ -613,6 +613,10 @@
#define AP_TRACE_ENABLE1
#define AP_TRACE_EXTENDED 2
int trace_enable;
+#define AP_MERGE_TRAILERS_UNSET0
+#define AP_MERGE_TRAILERS_ENABLE 1
+#define AP_MERGE_TRAILERS_DISABLE 2
+int merge_trailers;
} core_server_config;
Index: include/httpd.h
===
--- include/httpd.h (revision 1593540)
+++ include/httpd.h (working copy)
@@ -1006,6 +1006,11 @@
* record to improve 64bit alignment the next time we need to break
* binary compatibility for some other reason.
*/
+
+/** MIME trailer environment from the request */
+apr_table_t *trailers_in;
+/** MIME trailer environment from the response */
+apr_table_t *trailers_out;
};
/**
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (revision 1593540)
+++ modules/http/http_filters.c (working copy)
@@ -215,6 +215,7 @@
ap_input_mode_t mode, apr_read_type_e block,
apr_off_t readbytes)
{
+core_server_config *conf;
apr_bucket *e;
http_ctx_t *ctx = f-ctx;
apr_status_t rv;
@@ -222,6 +223,9 @@
int http_error = HTTP_REQUEST_ENTITY_TOO_LARGE;
apr_bucket_brigade *bb;
+conf = (core_server_config *)
+ap_get_module_config(f-r-server-module_config, core_module);
+
/* just get out of the way of things we don't want. */
if (mode != AP_MODE_READBYTES mode != AP_MODE_GETLINE) {
return ap_get_brigade(f-next, b, mode, block, readbytes);
@@ -403,13 +407,50 @@
}
if (!ctx-remaining) {
-/* Handle trailers by calling ap_get_mime_headers again! */
+int saved_status = f-r-status;
+apr_table_t *saved_headers_in = f-r-headers_in;
+const char *saved_error_notes = apr_table_get(f-r-notes,
+ error-notes);
+
+/* By default, don't merge in the trailers
+ * (treat UNSET as DISABLE) */
+if (conf-merge_trailers != AP_MERGE_TRAILERS_ENABLE) {
+f-r-status = HTTP_OK;
+f-r-headers_in = f-r-trailers_in;
+apr_table_clear(f-r-headers_in);
+}
+
ctx-state = BODY_NONE;
ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+if (f-r-status == HTTP_OK) {
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+
Re: stop copying footers to r-headers_in?
I did not realize your mail is 8 days old, I just received it -- gmail is having fun these days :( Sorry for this misunderstanding and the reply that goes along... Regards, Yann.
Re: stop copying footers to r-headers_in?
On Tue, May 06, 2014 at 07:45:42PM -0400, Eric Covener wrote:
On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic ylavic@gmail.com wrote:
This patch (still) does not propose to merge splitted trailers into
headers (for those broken by the change), but when (and why) would we
do that?
For 2.2, I would personally want to have the ability to restore the
old merging behavior before any release. Joe drew his line there as
well.
I hacked the attached up yesterday, before seeing Yann's mail, untested.
Yann, in your smaller patch is it deliberate that you only handle one of
the two places trailers are read in ap_http_filter()? Either that is an
oversight or I am missing something.
I'm also daunted by the size of change required to a rewrite core
function like ap_rgetline* to be non-blocking, at least in 2.[24]; that
seems really like an orthogonal issue.
Regards, Joe
Index: include/httpd.h
===
--- include/httpd.h (revision 1592743)
+++ include/httpd.h (working copy)
@@ -1032,6 +1032,10 @@
*/
apr_sockaddr_t *useragent_addr;
char *useragent_ip;
+
+/* MIME headers from the trailer from a chunked request,
+ * received only after the request body has been read. */
+apr_table_t *trailers_in;
};
/**
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (revision 1592743)
+++ modules/http/http_filters.c (working copy)
@@ -230,7 +230,29 @@
return get_remaining_chunk_line(ctx, b, linelimit);
}
+static apr_status_t read_chunked_trailers(http_ctx_t *ctx, ap_filter_t *f,
+ apr_bucket_brigade *b)
+{
+apr_bucket *e;
+apr_table_t *tmp;
+/* Handle trailers by calling ap_get_mime_headers again! */
+ctx-state = BODY_NONE;
+
+/* Temporarily fool ap_get_mime_headers() into using the trailers
+ * table. Ugly but simple. */
+tmp = f-r-headers_in;
+f-r-headers_in = f-r-trailers_in;
+ap_get_mime_headers(f-r);
+f-r-headers_in = tmp;
+
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(b, e);
+ctx-eos_sent = 1;
+
+return APR_SUCCESS;
+}
+
/* This is the HTTP_INPUT filter for HTTP requests and responses from
* proxied servers (mod_proxy). It handles chunked and content-length
* bodies. This can only be inserted/used after the headers
@@ -425,13 +447,7 @@
}
if (!ctx-remaining) {
-/* Handle trailers by calling ap_get_mime_headers again! */
-ctx-state = BODY_NONE;
-ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+return read_chunked_trailers(ctx, f, b);
}
}
}
@@ -534,13 +550,7 @@
}
if (!ctx-remaining) {
-/* Handle trailers by calling ap_get_mime_headers again! */
-ctx-state = BODY_NONE;
-ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+return read_chunked_trailers(ctx, f, b);
}
}
break;
Index: modules/http/http_request.c
===
--- modules/http/http_request.c (revision 1592743)
+++ modules/http/http_request.c (working copy)
@@ -463,6 +463,7 @@
new-main= r-main;
new-headers_in = r-headers_in;
+new-trailers_in = r-headers_in;
new-headers_out = apr_table_make(r-pool, 12);
if (ap_is_HTTP_REDIRECT(new-status)) {
const char *location = apr_table_get(r-headers_out, Location);
Index: server/protocol.c
===
--- server/protocol.c (revision 1592743)
+++ server/protocol.c (working copy)
@@ -921,6 +921,7 @@
r-headers_out = apr_table_make(r-pool, 12);
r-err_headers_out = apr_table_make(r-pool, 5);
r-notes = apr_table_make(r-pool, 5);
+r-trailers_in = apr_table_make(r-pool, 5);
r-request_config = ap_create_request_config(r-pool);
/* Must be set before we run create request hook */
@@ -1185,6 +1186,7 @@
rnew-status = HTTP_OK;
rnew-headers_in = apr_table_copy(rnew-pool, r-headers_in);
+rnew-trailers_in = apr_table_copy(rnew-pool, r-trailers_in);
/* did the original request have a body? (e.g. POST w/SSI tags)
* if so, make sure the subrequest doesn't inherit body headers
Re: stop copying footers to r-headers_in?
On Wed, May 7, 2014 at 11:47 AM, Joe Orton jor...@redhat.com wrote:
I hacked the attached up yesterday, before seeing Yann's mail, untested.
The patch looks good (against 2.4.x), but as I said to Edward, I think
r-status should be restored (at least, error-notes probably too)
when r-proxyreq == PROXYREQ_RESPONSE.
Indeed, if the response is read in one go (headers + body + trailers),
and read_chunked_trailers() fails, r-status will be modified to
something different from the orginal one (while the body will be
forwarded as is).
Otherwise if the headers are forwarded before the trailers, the change
on r-status won't reach the client but may still disrupt the logging.
Yann, in your smaller patch is it deliberate that you only handle one of
the two places trailers are read in ap_http_filter()? Either that is an
oversight or I am missing something.
My oversight, I was influenced by the trunk (longer) patch where there
is only one place (with the BODY_CHUNK_TRAILER state).
I'm also daunted by the size of change required to a rewrite core
function like ap_rgetline* to be non-blocking, at least in 2.[24]; that
seems really like an orthogonal issue.
The patch looks huge but actually this is mostly due to the
accompanying reindentation.
Below is the (latest) patch for ap_rgetline_core() =
ap_rgetline_ex(), with space changes ignored.
I don't find it so daunting, but I agree it is orthogonal since it
concerns non-blocking, ie. mpm_event, with which I am not sure the
trailers are read in non-blocking mode by any handler (although
ap_http_filter() can be called in non-blocking mode)...
+1 to leave it aside from 2.4.x for now, but I think we will probably
need a non-blocking ap_get_mime_headers_ex() (hence ap_rgetline_ex)
soon.
Regards,
Yann.
Index: server/protocol.c
===
--- server/protocol.c(revision 1594967)
+++ server/protocol.c(working copy)
@@ -193,13 +193,15 @@
* stricter protocol adherence and better input filter behavior during
* chunked trailer processing (for http).
*
- * If s is NULL, ap_rgetline_core will allocate necessary memory from r-pool.
+ * If s is NULL, ap_rgetline_ex will allocate necessary memory from r-pool.
*
* Returns APR_SUCCESS if there are no problems and sets *read to be
* the full length of s.
*
* APR_ENOSPC is returned if there is not enough buffer space.
- * Other errors may be returned on other errors.
+ * APR_EAGAIN is returned if a non-blocking read would have blocked, possibly
+ * with a partial line available in *s (and a *read 0).
+ * Other downstream errors may be returned.
*
* The LF is *not* returned in the buffer. Therefore, a *read of 0
* indicates that an empty line was read.
@@ -210,9 +212,10 @@
*If no LF is detected on the last line due to a dropped connection
*or a full buffer, that's considered an error.
*/
-AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
+AP_DECLARE(apr_status_t) ap_rgetline_ex(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
- int fold, apr_bucket_brigade *bb)
+int fold, apr_bucket_brigade *bb,
+apr_read_type_e block, ap_filter_t *f)
{
apr_status_t rv;
apr_bucket *e;
@@ -220,6 +223,10 @@
char *pos, *last_char = *s;
int do_alloc = (*s == NULL), saw_eos = 0;
+if (f == NULL) {
+f = r-proto_input_filters;
+}
+
/*
* Initialize last_char as otherwise a random value will be compared
* against APR_ASCII_LF at the end of the loop if bb only contains
@@ -230,8 +237,28 @@
for (;;) {
apr_brigade_cleanup(bb);
-rv = ap_get_brigade(r-proto_input_filters, bb, AP_MODE_GETLINE,
-APR_BLOCK_READ, 0);
+rv = ap_get_brigade(f, bb, AP_MODE_GETLINE, block, 0);
+if (block == APR_NONBLOCK_READ
+ (APR_STATUS_IS_EAGAIN(rv)
+|| (rv == APR_SUCCESS APR_BRIGADE_EMPTY(bb {
+*read = bytes_handled;
+if (*s) {
+/* ensure this string is NUL terminated */
+if (n bytes_handled + 1) {
+if (bytes_handled 0) {
+(*s)[bytes_handled-1] = '\0';
+}
+else {
+(*s)[0] = '\0';
+}
+return APR_ENOSPC;
+}
+(*s)[bytes_handled] = '\0';
+}
+if (rv == APR_SUCCESS) {
+rv = APR_EAGAIN;
+}
+}
if (rv != APR_SUCCESS) {
return rv;
}
@@ -287,7 +314,7 @@
/* We'll assume the common case where one bucket is enough. */
if (!*s) {
current_alloc = len;
-
Re: stop copying footers to r-headers_in?
On Mon, May 12, 2014 at 7:03 PM, Edward Lu chaos...@gmail.com wrote:
Both the things you caught were probably just errors I made in manually
merging the patch into 2.2.x; here's a revised version, just in case.
Looks good, but you did not address Eric's comment about parsing the
trailers directly into trailers_in and merge them later if required
(MergeTrailers on).
So imo :
f-r-status = HTTP_OK;
f-r-headers_in = f-r-trailers_in;
apr_table_clear(f-r-headers_in);
if (saved_error_notes) {
apr_table_unset(f-r-notes, error-notes);
}
should be used unconditionnally before calling ap_get_mime_headers(),
be it only for the relevance of status == HTTP_OK or the error-notes'
log after the call.
I'm not really clear on the error-notes discussion. It makes sense that we
wouldn't care too much if we don't save the ones from reading the headers,
since we would have errored earlier if they were set.
Agreed.
What happens currently
if there's an error reading the trailers and we are not merging the
trailers? Seems like the error-notes would just get overwritten with
nothing, in the general case, and they wouldn't be able to be used anywhere.
If reading/parsing the trailers fails on the client side, we still
want to respond with the correct (trailers) error-notes (eg. trailer
too large, too many trailers...), so the error-notes should not be
unset/restored before leaving in this case.
However ap_http_filter() is also called for mod_proxy_http's backend
responses (r-proxyreq == PROXYREQ_RESPONSE), and in that case we
don't want the trailers' error-notes to polute anything, so it should
be unset/restored before leaving.
The same apply for r-status, the value set by ap_get_mime_headers()
is relevant when handling a request but not for a response (where the
one from the status line received earlier has to be preserved).
So r-status should be restored before leaving only in the
PROXYREQ_RESPONSE case.
Re: stop copying footers to r-headers_in?
On May 6, 2014, at 7:45 PM, Eric Covener cove...@gmail.com wrote: On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic ylavic@gmail.com wrote: On Tue, May 6, 2014 at 1:41 PM, Eric Covener cove...@gmail.com wrote: I'd really like to start chucking trailers, but the scale of these patches really frightens me for 2.4 and 2.2 and accompanying what will likely be a security roll-up. Understood. Is anyone sitting on a more tactical version of the fix? Well, maybe I should stop proposing something here but maybe the attached patch can help. I think it's a tactical version of the previous one, in that it does *not* touch ap_get_mime_headers*() (nor ap_rgetline*()), but saves/restores the data (request's fields, notes) modified by the function when called from ap_http_filter(). It could be a way for 2.2.x, but I don't think it's enough for trunk or even 2.4.x, because this implementation still breaks non-blocking mode (hence event), which is (also) why the core functions were modified in the previous patch. I think I could be on board for that plan. Any others? +1 This patch (still) does not propose to merge splitted trailers into headers (for those broken by the change), but when (and why) would we do that? For 2.2, I would personally want to have the ability to restore the old merging behavior before any release. Joe drew his line there as well. But I think Ed Lu is lurking and can take that last bit on. -- Eric Covener cove...@gmail.com
Re: stop copying footers to r-headers_in?
Both the things you caught were probably just errors I made in manually
merging the patch into 2.2.x; here's a revised version, just in case.
Thanks for the review.
I'm not really clear on the error-notes discussion. It makes sense that we
wouldn't care too much if we don't save the ones from reading the headers,
since we would have errored earlier if they were set. What happens
currently if there's an error reading the trailers and we are not merging
the trailers? Seems like the error-notes would just get overwritten with
nothing, in the general case, and they wouldn't be able to be used anywhere.
On Sun, May 11, 2014 at 3:55 PM, Yann Ylavic ylavic@gmail.com wrote:
On Sun, May 11, 2014 at 6:35 PM, Yann Ylavic ylavic@gmail.com wrote:
Restoring the status and notes (set before the body is read) seems
unconditional to me.
Well, actually I'm not really sure whether we should preserve or
overwritte the error-notes here.
In most cases (but PROXYREQ_RESPONSE discussed below), we are still
reading the request.
If an error is raised by ap_get_mime_headers(), we probably want to
respond with a 4xx status and the corresponding error-notes (from this
call).
Moreover, the original error-notes (from the headers' parsing) is
very likely to be unset here (or we would have responded with that
error at that time).
However, when ap_http_filter() is called in PROXYREQ_RESPONSE mode, we
probably need to discard the forwarded response's trailers'
error-notes, since the client is not the culprit.
Maybe we should distinguish (at least) these 2 modes in the final patch.
Index: include/http_core.h
===
--- include/http_core.h (revision 1593540)
+++ include/http_core.h (working copy)
@@ -613,6 +613,10 @@
#define AP_TRACE_ENABLE1
#define AP_TRACE_EXTENDED 2
int trace_enable;
+#define AP_MERGE_TRAILERS_UNSET0
+#define AP_MERGE_TRAILERS_ENABLE 1
+#define AP_MERGE_TRAILERS_DISABLE 2
+int merge_trailers;
} core_server_config;
Index: include/httpd.h
===
--- include/httpd.h (revision 1593540)
+++ include/httpd.h (working copy)
@@ -1006,6 +1006,11 @@
* record to improve 64bit alignment the next time we need to break
* binary compatibility for some other reason.
*/
+
+/** MIME trailer environment from the request */
+apr_table_t *trailers_in;
+/** MIME trailer environment from the response */
+apr_table_t *trailers_out;
};
/**
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (revision 1593540)
+++ modules/http/http_filters.c (working copy)
@@ -215,6 +215,7 @@
ap_input_mode_t mode, apr_read_type_e block,
apr_off_t readbytes)
{
+core_server_config *conf;
apr_bucket *e;
http_ctx_t *ctx = f-ctx;
apr_status_t rv;
@@ -222,6 +223,9 @@
int http_error = HTTP_REQUEST_ENTITY_TOO_LARGE;
apr_bucket_brigade *bb;
+conf = (core_server_config *)
+ap_get_module_config(f-r-server-module_config, core_module);
+
/* just get out of the way of things we don't want. */
if (mode != AP_MODE_READBYTES mode != AP_MODE_GETLINE) {
return ap_get_brigade(f-next, b, mode, block, readbytes);
@@ -403,13 +407,50 @@
}
if (!ctx-remaining) {
-/* Handle trailers by calling ap_get_mime_headers again! */
+int saved_status = f-r-status;
+apr_table_t *saved_headers_in = f-r-headers_in;
+const char *saved_error_notes = apr_table_get(f-r-notes,
+ error-notes);
+
+/* By default, don't merge in the trailers
+ * (treat UNSET as DISABLE) */
+if (conf-merge_trailers != AP_MERGE_TRAILERS_ENABLE) {
+f-r-status = HTTP_OK;
+f-r-headers_in = f-r-trailers_in;
+apr_table_clear(f-r-headers_in);
+}
+
ctx-state = BODY_NONE;
ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+if (f-r-status == HTTP_OK) {
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(b, e);
+ctx-eos_sent = 1;
+rv = APR_SUCCESS;
+}
+else {
+const char *error_notes = apr_table_get(f-r-notes,
+error-notes);
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f-r,
+ Error while reading HTTP
Re: stop copying footers to r-headers_in?
Here's a 2.2.x backport of Yann's patch with a directive, MergeTrailers,
added in to opt-in to the old behavior. By default, it doesn't merge the
trailers. I'm working on adding the capability to mod_log_config to log the
trailers, but I figured I'd post this patch first to get some review.
- Ed
On Tue, May 6, 2014 at 7:45 PM, Eric Covener cove...@gmail.com wrote:
On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic ylavic@gmail.com wrote:
On Tue, May 6, 2014 at 1:41 PM, Eric Covener cove...@gmail.com wrote:
I'd really like to start chucking trailers, but the scale of these
patches really frightens me for 2.4 and 2.2 and accompanying what will
likely be a security roll-up.
Understood.
Is anyone sitting on a more tactical version of the fix?
Well, maybe I should stop proposing something here but maybe the
attached patch can help.
I think it's a tactical version of the previous one, in that it does
*not* touch ap_get_mime_headers*() (nor ap_rgetline*()), but
saves/restores the data (request's fields, notes) modified by the
function when called from ap_http_filter().
It could be a way for 2.2.x, but I don't think it's enough for trunk
or even 2.4.x, because this implementation still breaks non-blocking
mode (hence event), which is (also) why the core functions were
modified in the previous patch.
I think I could be on board for that plan. Any others?
This patch (still) does not propose to merge splitted trailers into
headers (for those broken by the change), but when (and why) would we
do that?
For 2.2, I would personally want to have the ability to restore the
old merging behavior before any release. Joe drew his line there as
well.
But I think Ed Lu is lurking and can take that last bit on.
--
Eric Covener
cove...@gmail.com
Index: include/http_core.h
===
--- include/http_core.h (revision 1593540)
+++ include/http_core.h (working copy)
@@ -613,6 +613,10 @@
#define AP_TRACE_ENABLE1
#define AP_TRACE_EXTENDED 2
int trace_enable;
+#define AP_MERGE_TRAILERS_UNSET0
+#define AP_MERGE_TRAILERS_ENABLE 1
+#define AP_MERGE_TRAILERS_DISABLE 2
+int merge_trailers;
} core_server_config;
Index: include/httpd.h
===
--- include/httpd.h (revision 1593540)
+++ include/httpd.h (working copy)
@@ -1006,6 +1006,11 @@
* record to improve 64bit alignment the next time we need to break
* binary compatibility for some other reason.
*/
+
+/** MIME trailer environment from the request */
+apr_table_t *trailers_in;
+/** MIME trailer environment from the response */
+apr_table_t *trailers_out;
};
/**
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (revision 1593540)
+++ modules/http/http_filters.c (working copy)
@@ -215,6 +215,7 @@
ap_input_mode_t mode, apr_read_type_e block,
apr_off_t readbytes)
{
+core_server_config *conf;
apr_bucket *e;
http_ctx_t *ctx = f-ctx;
apr_status_t rv;
@@ -222,6 +223,9 @@
int http_error = HTTP_REQUEST_ENTITY_TOO_LARGE;
apr_bucket_brigade *bb;
+conf = (core_server_config *)
+ap_get_module_config(f-r-server-module_config, core_module);
+
/* just get out of the way of things we don't want. */
if (mode != AP_MODE_READBYTES mode != AP_MODE_GETLINE) {
return ap_get_brigade(f-next, b, mode, block, readbytes);
@@ -403,13 +407,50 @@
}
if (!ctx-remaining) {
-/* Handle trailers by calling ap_get_mime_headers again! */
+int saved_status = f-r-status;
+apr_table_t *saved_headers_in = f-r-headers_in;
+const char *saved_error_notes = apr_table_get(f-r-notes,
+ error-notes);
+
+/* By default, don't merge in the trailers
+ * (treat UNSET as DISABLE) */
+if (conf-merge_trailers != AP_MERGE_TRAILERS_ENABLE) {
+f-r-status = HTTP_OK;
+f-r-headers_in = f-r-trailers_in;
+apr_table_clear(f-r-headers_in);
+}
+
ctx-state = BODY_NONE;
ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+if (f-r-status == HTTP_OK) {
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(b, e);
+ctx-eos_sent = 1;
+rv = APR_SUCCESS;
+}
+else {
+const char *error_notes =
Re: stop copying footers to r-headers_in?
On Wed, May 7, 2014 at 6:35 AM, Ruediger Pluem rpl...@apache.org wrote: Configurable by an option? If yes, what is the default behaviour? my 2 cents: Default, collect them in r-trailers_in where not much can be done with them. With a directive, restore the tricky historical behavior (append them to r-headers_in) -- Eric Covener cove...@gmail.com
Re: stop copying footers to r-headers_in?
On Fri, May 9, 2014 at 5:01 PM, Edward Lu chaos...@gmail.com wrote:
Here's a 2.2.x backport of Yann's patch with a directive, MergeTrailers,
added in to opt-in to the old behavior. By default, it doesn't merge the
trailers. I'm working on adding the capability to mod_log_config to log the
trailers, but I figured I'd post this patch first to get some review.
(paste from your patch with comments below)
Index: modules/http/http_request.c
===
--- modules/http/http_request.c(revision 1593540)
+++ modules/http/http_request.c(working copy)
@@ -384,8 +384,10 @@
new-main= r-main;
new-headers_in = r-headers_in;
+new-trailers_in = r-trailers_in;
new-headers_out = apr_table_make(r-pool, 12);
new-err_headers_out = r-err_headers_out;
+new-trailers_out= r-trailers_out;
Here my patch used :
new-trailers_out = apr_table_make(r-pool, 5);
so to start internal redirect(s) with fresh new outgoing trailers.
I think the previous body (if any) is likely to be consumed when
ap_internal_redirect[_handler] is called.
I don't know what's the correct thing to do, though, my understanding
is that err_headers_out are preserved here because they may contain
(set-)cookies or alike, but the final response will be the one of the
redirect (headers and body, if any).
new-subprocess_env = rename_original_env(r-pool, r-subprocess_env);
new-notes = apr_table_make(r-pool, 5);
@@ -495,6 +497,8 @@
r-headers_out);
r-err_headers_out = apr_table_overlay(r-pool, rr-err_headers_out,
r-err_headers_out);
+r-err_headers_out = apr_table_overlay(r-pool, rr-trailers_out,
+ r-trailers_out);
Probably a typo here, should be r-trailers_out =
Otherwise, I agree with Eric that ap_http_filter() should save and
restore the request's status/headers/notes by default, and only merge
the trailers if configured to.
Restoring the status and notes (set before the body is read) seems
unconditional to me.
Something like the following patch (against 2.2.x), which also
fixes/improves (compared to my previous patch) the unsetting of the
error-notes before/after the call to ap_get_mime_headers().
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c(revision 1593786)
+++ modules/http/http_filters.c(working copy)
@@ -403,13 +407,61 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
}
if (!ctx-remaining) {
-/* Handle trailers by calling ap_get_mime_headers again! */
+/* Handle trailers by calling ap_get_mime_headers again,
+ * but preserve original status and incoming headers (unless
+ * MergeTrailers in on) by saving/restoring them before/after.
+ */
+const char *error_notes = NULL;
+int saved_status = f-r-status;
+apr_table_t *saved_headers_in = f-r-headers_in;
+const char *saved_error_notes = apr_table_get(f-r-notes,
+ error-notes);
+
+f-r-status = HTTP_OK;
+f-r-headers_in = f-r-trailers_in;
+apr_table_clear(f-r-headers_in);
+if (saved_error_notes) {
+apr_table_unset(f-r-notes, error-notes);
+}
+
ctx-state = BODY_NONE;
ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+if (f-r-status == HTTP_OK) {
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(b, e);
+ctx-eos_sent = 1;
+rv = APR_SUCCESS;
+}
+else {
+error_notes = apr_table_get(f-r-notes, error-notes);
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f-r,
+ Error while reading HTTP trailer: %i%s%s,
+ f-r-status, error_notes ? : : ,
+ error_notes ? error_notes : );
+rv = APR_EINVAL;
+}
+
+/* By default, don't merge in the trailers
+ * (treat UNSET as DISABLE) */
+if (conf-merge_trailers != AP_MERGE_TRAILERS_ENABLE) {
+f-r-headers_in = saved_headers_in;
+}
+else {
+f-r-headers_in = apr_table_overlay(f-r-pool,
+
Re: stop copying footers to r-headers_in?
On Sun, May 11, 2014 at 6:35 PM, Yann Ylavic ylavic@gmail.com wrote: Restoring the status and notes (set before the body is read) seems unconditional to me. Well, actually I'm not really sure whether we should preserve or overwritte the error-notes here. In most cases (but PROXYREQ_RESPONSE discussed below), we are still reading the request. If an error is raised by ap_get_mime_headers(), we probably want to respond with a 4xx status and the corresponding error-notes (from this call). Moreover, the original error-notes (from the headers' parsing) is very likely to be unset here (or we would have responded with that error at that time). However, when ap_http_filter() is called in PROXYREQ_RESPONSE mode, we probably need to discard the forwarded response's trailers' error-notes, since the client is not the culprit. Maybe we should distinguish (at least) these 2 modes in the final patch.
Re: stop copying footers to r-headers_in?
Eric Covener wrote: On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic ylavic@gmail.com wrote: On Tue, May 6, 2014 at 1:41 PM, Eric Covener cove...@gmail.com wrote: I'd really like to start chucking trailers, but the scale of these patches really frightens me for 2.4 and 2.2 and accompanying what will likely be a security roll-up. Understood. Is anyone sitting on a more tactical version of the fix? Well, maybe I should stop proposing something here but maybe the attached patch can help. I think it's a tactical version of the previous one, in that it does *not* touch ap_get_mime_headers*() (nor ap_rgetline*()), but saves/restores the data (request's fields, notes) modified by the function when called from ap_http_filter(). It could be a way for 2.2.x, but I don't think it's enough for trunk or even 2.4.x, because this implementation still breaks non-blocking mode (hence event), which is (also) why the core functions were modified in the previous patch. I think I could be on board for that plan. Any others? +1 This patch (still) does not propose to merge splitted trailers into headers (for those broken by the change), but when (and why) would we do that? For 2.2, I would personally want to have the ability to restore the old merging behavior before any release. Joe drew his line there as well. Configurable by an option? If yes, what is the default behaviour? Regards Rüdiger
Re: stop copying footers to r-headers_in?
I'd really like to start chucking trailers, but the scale of these patches really frightens me for 2.4 and 2.2 and accompanying what will likely be a security roll-up. Is anyone sitting on a more tactical version of the fix?
Re: stop copying footers to r-headers_in?
On Tue, May 6, 2014 at 1:41 PM, Eric Covener cove...@gmail.com wrote:
I'd really like to start chucking trailers, but the scale of these
patches really frightens me for 2.4 and 2.2 and accompanying what will
likely be a security roll-up.
Understood.
Is anyone sitting on a more tactical version of the fix?
Well, maybe I should stop proposing something here but maybe the
attached patch can help.
I think it's a tactical version of the previous one, in that it does
*not* touch ap_get_mime_headers*() (nor ap_rgetline*()), but
saves/restores the data (request's fields, notes) modified by the
function when called from ap_http_filter().
It could be a way for 2.2.x, but I don't think it's enough for trunk
or even 2.4.x, because this implementation still breaks non-blocking
mode (hence event), which is (also) why the core functions were
modified in the previous patch.
This patch (still) does not propose to merge splitted trailers into
headers (for those broken by the change), but when (and why) would we
do that?
Index: server/protocol.c
===
--- server/protocol.c (revision 1592855)
+++ server/protocol.c (working copy)
@@ -1010,9 +1010,11 @@ request_rec *ap_read_request(conn_rec *conn)
r-allowed_methods = ap_make_method_list(p, 2);
r-headers_in = apr_table_make(r-pool, 25);
+r-trailers_in = apr_table_make(r-pool, 5);
r-subprocess_env = apr_table_make(r-pool, 25);
r-headers_out = apr_table_make(r-pool, 12);
r-err_headers_out = apr_table_make(r-pool, 5);
+r-trailers_out= apr_table_make(r-pool, 5);
r-notes = apr_table_make(r-pool, 5);
r-request_config = ap_create_request_config(r-pool);
@@ -1289,6 +1291,7 @@ AP_DECLARE(void) ap_set_sub_req_protocol(request_r
rnew-status = HTTP_OK;
rnew-headers_in = apr_table_copy(rnew-pool, r-headers_in);
+rnew-trailers_in = apr_table_copy(rnew-pool, r-trailers_in);
/* did the original request have a body? (e.g. POST w/SSI tags)
* if so, make sure the subrequest doesn't inherit body headers
@@ -1300,6 +1303,7 @@ AP_DECLARE(void) ap_set_sub_req_protocol(request_r
rnew-subprocess_env = apr_table_copy(rnew-pool, r-subprocess_env);
rnew-headers_out = apr_table_make(rnew-pool, 5);
rnew-err_headers_out = apr_table_make(rnew-pool, 5);
+rnew-trailers_out= apr_table_make(rnew-pool, 5);
rnew-notes = apr_table_make(rnew-pool, 5);
rnew-expecting_100 = r-expecting_100;
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (revision 1592855)
+++ modules/http/http_filters.c (working copy)
@@ -396,11 +396,41 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
again = 1; /* come around again */
if (ctx-state == BODY_CHUNK_TRAILER) {
+int saved_status = f-r-status;
+apr_table_t *saved_headers_in = f-r-headers_in;
+const char *saved_error_notes = apr_table_get(f-r-notes,
+ error-notes);
+
+f-r-status = HTTP_OK;
+f-r-headers_in = f-r-trailers_in;
+apr_table_clear(f-r-headers_in);
ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+if (f-r-status == HTTP_OK) {
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(b, e);
+ctx-eos_sent = 1;
+rv = APR_SUCCESS;
+}
+else {
+const char *error_notes = apr_table_get(f-r-notes,
+error-notes);
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f-r, APLOGNO()
+ Error while reading HTTP trailer: %i%s%s,
+ f-r-status, error_notes ? : : ,
+ error_notes ? error_notes : );
+rv = APR_EINVAL;
+}
+
+f-r-status = saved_status;
+f-r-headers_in = saved_headers_in;
+if (saved_error_notes) {
+apr_table_setn(f-r-notes, error-notes,
+ saved_error_notes);
+}
+else {
+apr_table_unset(f-r-notes, error-notes);
+}
+return rv;
}
break;
Index: modules/http/http_request.c
===
--- modules/http/http_request.c (revision 1592855)
+++ modules/http/http_request.c (working copy)
@@
Re: stop copying footers to r-headers_in?
On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic ylavic@gmail.com wrote: On Tue, May 6, 2014 at 1:41 PM, Eric Covener cove...@gmail.com wrote: I'd really like to start chucking trailers, but the scale of these patches really frightens me for 2.4 and 2.2 and accompanying what will likely be a security roll-up. Understood. Is anyone sitting on a more tactical version of the fix? Well, maybe I should stop proposing something here but maybe the attached patch can help. I think it's a tactical version of the previous one, in that it does *not* touch ap_get_mime_headers*() (nor ap_rgetline*()), but saves/restores the data (request's fields, notes) modified by the function when called from ap_http_filter(). It could be a way for 2.2.x, but I don't think it's enough for trunk or even 2.4.x, because this implementation still breaks non-blocking mode (hence event), which is (also) why the core functions were modified in the previous patch. I think I could be on board for that plan. Any others? This patch (still) does not propose to merge splitted trailers into headers (for those broken by the change), but when (and why) would we do that? For 2.2, I would personally want to have the ability to restore the old merging behavior before any release. Joe drew his line there as well. But I think Ed Lu is lurking and can take that last bit on. -- Eric Covener cove...@gmail.com
Re: stop copying footers to r-headers_in?
Resurrecting this thread. Yann, anything holding this one back?
On Mon, Oct 28, 2013 at 9:43 AM, Yann Ylavic ylavic@gmail.com wrote:
The declaration of ap_get_mime_headers_from() is falsy in the previous
patch, you should read instead :
+AP_DECLARE(apr_status_t)
+ap_get_mime_headers_from(request_rec *r, apr_bucket_brigade *bb,
+ ap_filter_t *from, apr_read_type_e block,
+ apr_table_t *to, ap_mime_headers_ctx_t **ctx)
+{
+return ap_parse_mime_headers(r, bb, from, block, to, ctx, NULL, NULL);
+}
+
--
Eric Covener
cove...@gmail.com
Re: stop copying footers to r-headers_in?
Sorry I stopped this along the way...
IIRC, my latest patch (not posted yet) does all the necessary in
ap_rgetline_ex() and ap_get_mime_headers_ex() (ie. parameters for the
input filter to read from and handling of the blocking mode), parses
the trailers in ap_http_filter() where they are read, defines and runs
the new trailer_parser hooks after the parsing, and does all the proxy
forwarding (in and out).
The missing points are the merge in r-headers_in based on a new
directive (for compatibility) and the mod_headers/mod_log_config
stuff.
Let me take a fresh look at the patch and finalize it, I come back very soon.
Regards,
Yann.
On Tue, Apr 1, 2014 at 3:36 PM, Eric Covener cove...@gmail.com wrote:
Resurrecting this thread. Yann, anything holding this one back?
On Mon, Oct 28, 2013 at 9:43 AM, Yann Ylavic ylavic@gmail.com wrote:
The declaration of ap_get_mime_headers_from() is falsy in the previous
patch, you should read instead :
+AP_DECLARE(apr_status_t)
+ap_get_mime_headers_from(request_rec *r, apr_bucket_brigade *bb,
+ ap_filter_t *from, apr_read_type_e block,
+ apr_table_t *to, ap_mime_headers_ctx_t **ctx)
+{
+return ap_parse_mime_headers(r, bb, from, block, to, ctx, NULL,
NULL);
+}
+
--
Eric Covener
cove...@gmail.com
Re: stop copying footers to r-headers_in?
On Tue, Apr 1, 2014 at 8:00 PM, Yann Ylavic ylavic@gmail.com wrote:
+static int parse_mime_headers(request_rec *r,
+ apr_bucket_brigade *bb,
+ apr_read_type_e block,
+ ap_filter_t *f, apr_table_t *t,
+ int *status, const char **html_notes,
+ ap_mime_state_t **state)
{
-char *last_field = NULL;
-apr_size_t last_len = 0;
-apr_size_t alloc_len = 0;
char *field;
char *value;
apr_size_t len;
-int fields_read = 0;
char *tmp_field;
+ap_mime_state_t x, *s;
core_server_config *conf =
ap_get_core_module_config(r-server-module_config);
+if (!state) {
+s = x;
+memset(s, 0, sizeof *s);
Oups, read memset(s, 0, sizeof *s) here.
+}
+else if (!(s = *state)) {
+*state = s = apr_pcalloc(r-pool, sizeof(ap_mime_state_t));
+}
+else if (s-done) {
+return APR_EOF;
+}
+
Re: stop copying footers to r-headers_in?
On Tue, Apr 1, 2014 at 2:00 PM, Yann Ylavic ylavic@gmail.com wrote: Here is the corresponding patch. I don't know if it's your mail client or mine (gmail), your patches always come through as both an attachment and in the body. Usually for anything more than a few lines, attachment-only is best. Can you say roughly how much it has changed, I was largely through a review of the previous patch. -- Eric Covener cove...@gmail.com
Re: stop copying footers to r-headers_in?
Hello Yann,
Comment included below.
Thanks,
Mike Rumph
On 4/1/2014 11:00 AM, Yann Ylavic wrote:
Index: modules/examples/mod_example_hooks.c
===
--- modules/examples/mod_example_hooks.c(revision 1583714)
+++ modules/examples/mod_example_hooks.c(working copy)
@@ -1227,7 +1227,23 @@ static int x_header_parser(request_rec *r)
return DECLINED;
}
+/*
+ * this routine gives our module another chance to examine the request
+ * trailers and to take special action.
+ *
+ * This is a RUN_ALL hook.
+ */
+static int x_trailer_parser(request_rec *r)
+{
+/*
+ * We don't actually *do* anything here, except note the fact that we were
+ * called.
+ */
+trace_request(r, x_trailer_parser());
+return DECLINED;
+}
+
/*
* This routine is called to check for any module-specific restrictions placed
* upon the requested resource. (See the mod_access_compat module for an
@@ -1469,6 +1485,7 @@ static void x_register_hooks(apr_pool_t *p)
ap_hook_translate_name(x_translate_name, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_map_to_storage(x_map_to_storage, NULL,NULL, APR_HOOK_MIDDLE);
ap_hook_header_parser(x_header_parser, NULL, NULL, APR_HOOK_MIDDLE);
+ap_hook_header_parser(x_trailer_parser, NULL, NULL, APR_HOOK_MIDDLE);
Should this be ap_hook_trailer_parser?
ap_hook_fixups(x_fixups, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_type_checker(x_type_checker, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_check_access(x_check_access, NULL, NULL, APR_HOOK_MIDDLE,
Re: stop copying footers to r-headers_in?
On Tue, Apr 1, 2014 at 8:36 PM, Eric Covener cove...@gmail.com wrote: On Tue, Apr 1, 2014 at 2:00 PM, Yann Ylavic ylavic@gmail.com wrote: Here is the corresponding patch. I don't know if it's your mail client or mine (gmail), your patches always come through as both an attachment and in the body. Usually for anything more than a few lines, attachment-only is best. It's me, I though some prefer patches in the body while others as attachment. I will use only attachments for such big diff next times. Can you say roughly how much it has changed, I was largely through a review of the previous patch. Actually, aside from funcs/vars/params renaming, the only difference is ap_proxy_fill_hdrbrgd() and its use in stream_reqbody_chunked(), so that trailers_in are forwarded. All the remaining was already there in the previous patch. I plan now to implement the RFC checks in a trailer_parser (really last) hook, is this the way to go?
Re: stop copying footers to r-headers_in?
Hi Mike,
On Tue, Apr 1, 2014 at 9:21 PM, Mike Rumph mike.ru...@oracle.com wrote:
Comment included below.
On 4/1/2014 11:00 AM, Yann Ylavic wrote:
Index: modules/examples/mod_example_hooks.c
===
--- modules/examples/mod_example_hooks.c(revision 1583714)
+++ modules/examples/mod_example_hooks.c(working copy)
@@ -1227,7 +1227,23 @@ static int x_header_parser(request_rec *r)
return DECLINED;
}
+/*
+ * this routine gives our module another chance to examine the request
+ * trailers and to take special action.
+ *
+ * This is a RUN_ALL hook.
+ */
+static int x_trailer_parser(request_rec *r)
+{
+/*
+ * We don't actually *do* anything here, except note the fact that we
were
+ * called.
+ */
+trace_request(r, x_trailer_parser());
+return DECLINED;
+}
+
/*
* This routine is called to check for any module-specific restrictions
placed
* upon the requested resource. (See the mod_access_compat module for
an
@@ -1469,6 +1485,7 @@ static void x_register_hooks(apr_pool_t *p)
ap_hook_translate_name(x_translate_name, NULL, NULL,
APR_HOOK_MIDDLE);
ap_hook_map_to_storage(x_map_to_storage, NULL,NULL,
APR_HOOK_MIDDLE);
ap_hook_header_parser(x_header_parser, NULL, NULL, APR_HOOK_MIDDLE);
+ap_hook_header_parser(x_trailer_parser, NULL, NULL, APR_HOOK_MIDDLE);
Should this be ap_hook_trailer_parser?
It should, thanks.
Will be fixed in the next patch.
ap_hook_fixups(x_fixups, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_type_checker(x_type_checker, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_check_access(x_check_access, NULL, NULL, APR_HOOK_MIDDLE,
Re: stop copying footers to r-headers_in?
On Tue, Apr 1, 2014 at 8:00 PM, Yann Ylavic ylavic@gmail.com wrote:
Here is the corresponding patch.
+static int parse_mime_headers(request_rec *r,
+ apr_bucket_brigade *bb,
+ apr_read_type_e block,
+ ap_filter_t *f, apr_table_t *t,
+ int *status, const char **html_notes,
+ ap_mime_state_t **state)
{
[snip]
@@ -963,20 +1054,60 @@ static int field_name_len(const char *field)
* has been updated already.)
*/
if (!folded) {
-last_field = field;
-last_len = len;
+s-last_field = field;
+s-last_len = len;
}
+
+/* We may still be EAGAIN here */
+if (rv != APR_SUCCESS) {
+AP_DEBUG_ASSERT(APR_STATUS_IS_EAGAIN(rv));
+s-fold = 1;
+return rv;
+}
+
+/* Found a blank line, stop. */
+if (len == 0) {
+s-done = 1;
+break;
+}
I missed to remove this same check in the original code just before
if (!folded) above.
Fixed for the next patch already.
Re: stop copying footers to r-headers_in?
On Tue, Apr 1, 2014 at 11:35 PM, Yann Ylavic ylavic@gmail.com wrote:
On Tue, Apr 1, 2014 at 8:36 PM, Eric Covener cove...@gmail.com wrote:
Can you say roughly how much it has changed, I was largely through a review
of the previous patch.
Actually, aside from funcs/vars/params renaming, the only difference
is ap_proxy_fill_hdrbrgd() and its use in stream_reqbody_chunked(), so
that trailers_in are forwarded.
All the remaining was already there in the previous patch.
Attached is the diff between the 2 patches applied.
--- ../trunk-r1536297/server/protocol.c 2014-04-01 22:32:53.0 +0200
+++ server/protocol.c 2014-04-01 22:32:36.0 +0200
@@ -193,12 +193,13 @@ AP_DECLARE(apr_time_t) ap_rationalize_mt
* stricter protocol adherence and better input filter behavior during
* chunked trailer processing (for http).
*
- * If s is NULL, ap_rgetline_from will allocate necessary memory from r-pool.
+ * If s is NULL, ap_rgetline_ex will allocate necessary memory from r-pool.
*
* Returns APR_SUCCESS if there are no problems and sets *read to be
* the full length of s.
*
* APR_ENOSPC is returned if there is not enough buffer space.
+ * APR_EAGAIN is returned if a non-blocking read would have blocked.
* Other errors may be returned on other errors.
*
* The LF is *not* returned in the buffer. Therefore, a *read of 0
@@ -210,11 +211,10 @@ AP_DECLARE(apr_time_t) ap_rationalize_mt
*If no LF is detected on the last line due to a dropped connection
*or a full buffer, that's considered an error.
*/
-AP_DECLARE(apr_status_t) ap_rgetline_from(char **s, apr_size_t n,
- apr_size_t *read, request_rec *r,
- int fold, apr_bucket_brigade *bb,
- ap_filter_t *from,
- apr_read_type_e block)
+AP_DECLARE(apr_status_t) ap_rgetline_ex(char **s, apr_size_t n,
+apr_size_t *read, request_rec *r,
+int fold, apr_bucket_brigade *bb,
+apr_read_type_e block, ap_filter_t *f)
{
apr_status_t rv;
apr_bucket *e;
@@ -222,8 +222,8 @@ AP_DECLARE(apr_status_t) ap_rgetline_fro
char *pos, *last_char = *s;
int do_alloc = (*s == NULL), saw_eos = 0;
-if (from == NULL) {
-from = r-proto_input_filters;
+if (f == NULL) {
+f = r-proto_input_filters;
}
/*
@@ -236,10 +236,10 @@ AP_DECLARE(apr_status_t) ap_rgetline_fro
for (;;) {
apr_brigade_cleanup(bb);
-rv = ap_get_brigade(from, bb, AP_MODE_GETLINE, block, 0);
+rv = ap_get_brigade(f, bb, AP_MODE_GETLINE, block, 0);
if (block == APR_NONBLOCK_READ
- ((rv == APR_SUCCESS APR_BRIGADE_EMPTY(bb))
-|| APR_STATUS_IS_EAGAIN(rv))) {
+ (APR_STATUS_IS_EAGAIN(rv)
+|| (rv == APR_SUCCESS APR_BRIGADE_EMPTY(bb {
*read = bytes_handled;
if (*s) {
/* ensure this string is NUL terminated */
@@ -371,10 +371,10 @@ AP_DECLARE(apr_status_t) ap_rgetline_fro
apr_brigade_cleanup(bb);
/* We only care about the first byte. */
-rv = ap_get_brigade(from, bb, AP_MODE_SPECULATIVE, block, 1);
+rv = ap_get_brigade(f, bb, AP_MODE_SPECULATIVE, block, 1);
if (block == APR_NONBLOCK_READ
- ((rv == APR_SUCCESS APR_BRIGADE_EMPTY(bb))
-|| APR_STATUS_IS_EAGAIN(rv))) {
+ (APR_STATUS_IS_EAGAIN(rv)
+|| (rv == APR_SUCCESS APR_BRIGADE_EMPTY(bb {
if (rv == APR_SUCCESS) {
rv = APR_EAGAIN;
}
@@ -435,26 +435,27 @@ AP_DECLARE(apr_status_t) ap_rgetline_fro
next_size = n - bytes_handled;
-rv = ap_rgetline_from(tmp, next_size, next_len, r, 0, bb,
- from, block);
-if ((rv == APR_SUCCESS || (block == APR_NONBLOCK_READ
- APR_STATUS_IS_EAGAIN(rv)))
+rv = ap_rgetline_ex(tmp, next_size, next_len, r, 0, bb,
+block, f);
+if ((rv == APR_SUCCESS
+|| (block == APR_NONBLOCK_READ
+ APR_STATUS_IS_EAGAIN(rv)))
(next_len 0)) {
if (do_alloc) {
-char *new_buffer;
-apr_size_t new_size = bytes_handled + next_len + 1;
+char *new_buffer;
+apr_size_t new_size = bytes_handled + next_len + 1;
-/* we need to alloc an extra byte for a null
Re: stop copying footers to r-headers_in?
The declaration of ap_get_mime_headers_from() is falsy in the previous
patch, you should read instead :
+AP_DECLARE(apr_status_t)
+ap_get_mime_headers_from(request_rec *r, apr_bucket_brigade *bb,
+ ap_filter_t *from, apr_read_type_e block,
+ apr_table_t *to, ap_mime_headers_ctx_t **ctx)
+{
+return ap_parse_mime_headers(r, bb, from, block, to, ctx, NULL, NULL);
+}
+
Re: stop copying footers to r-headers_in?
On Tue, Oct 22, 2013 at 2:18 PM, Yann Ylavic ylavic@gmail.com wrote:
-AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
apr_bucket_brigade *bb)
+struct ap_mime_headers_ctx_t {
+int fields_read;
+char *last_field;
+apr_size_t last_len;
+apr_size_t alloc_len;
+unsigned int fold :1,
+ done :1;
+};
+
+AP_DECLARE(apr_status_t)
+ap_get_mime_headers_ex(request_rec *r, apr_bucket_brigade *bb, apr_table_t
*to,
+ ap_filter_t *from, apr_read_type_e block,
+ ap_mime_headers_ctx_t **pctx)
{
-char *last_field = NULL;
-apr_size_t last_len = 0;
-apr_size_t alloc_len = 0;
Making my way through the review, had a tough time following the need
for this part. Any pointers?
Re: stop copying footers to r-headers_in?
On Mon, Oct 28, 2013 at 2:23 AM, Eric Covener cove...@gmail.com wrote:
On Tue, Oct 22, 2013 at 2:18 PM, Yann Ylavic ylavic@gmail.com wrote:
-AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
apr_bucket_brigade *bb)
+struct ap_mime_headers_ctx_t {
+int fields_read;
+char *last_field;
+apr_size_t last_len;
+apr_size_t alloc_len;
+unsigned int fold :1,
+ done :1;
+};
+
+AP_DECLARE(apr_status_t)
+ap_get_mime_headers_ex(request_rec *r, apr_bucket_brigade *bb,
apr_table_t
*to,
+ ap_filter_t *from, apr_read_type_e block,
+ ap_mime_headers_ctx_t **pctx)
{
-char *last_field = NULL;
-apr_size_t last_len = 0;
-apr_size_t alloc_len = 0;
Making my way through the review, had a tough time following the need
for this part. Any pointers?
I first simply created ap_get_mime_headers_ex() with the new destination
table argument to so that ap_http_filter() would use with r-trailers_in
instead of r-headers_in.
Then I realized that as well as adding yet another ap_get_mime_headers*()
function, this one had to be prepared to handle non-blocking mode, like
ap_http_filter() is ... but with the trailers (which are currently read in
blocking mode whatever ap_http_filter() calling mode is).
So I added ap_mime_headers_ctx_t to make ap_get_mime_headers_ex()
reentrant (with a given context, which ap_http_filter() can associate with
its own context).
ap_http_filter() is now able to handle non-blocking trailers in its state
machine (eg. in the corresponding BODY_CHUNK_TRAILER state), but since
ap_get_mime_headers() would ap_get_brigade(r-proto_input_filters), that
would end up with an infinite recursion between ap_http_filter() and
ap_get_mime_headers(). Hence the ap_get_mime_headers_ex()'s argument
from, which aims to receive ap_http_filter()-next...
Re: stop copying footers to r-headers_in?
On Thu, Oct 24, 2013 at 7:42 PM, Eric Covener cove...@gmail.com wrote: On Wed, Oct 23, 2013 at 8:04 AM, Yann Ylavic ylavic@gmail.com wrote: 1) add r-footers_in and use it in 2.2 and up by default Do that mean no API/ABI change ? In the sense that it needs to be backportable, yes -- but it will mean a behavior change to existing APIs. Depends on how you interpret it I guess, but I think the confusion over trailers/headers is something that needs to be forcible corrected in those service releases. Concretely, is request_rec::trailers_in/out (at the very end of the struct) something backportable or should the footers be stored elsewhere? According to [my understanding of] the rfc2616 and/or draft-ietf-httpbis-p1-messaging-24 about the HTTP trailer (chunking, TE and Trailer sections), they are hop-by-hop (anyway negociable/negociated hop-by-hop). They could then be stored in something more related to the request's connection(s), for *example* in the http_ctx_t used by ap_http_filter for input, which could probably be shared with protocol output filters too for the trailers_out. There is still the need to access them from r, for *example* (still) with something like the request_config of the http_module. But this looks like a big hack compared to the (simple) r-trailers_in/out... So, is the backportability something to care about for trunk (now) or things should be kept [as] simple [as possible] there and complications arise when backporting (using a different storage/access for example)? My plans were to use request_rec for now and change this later if it oughts to, but it could be useful to point/suggest me a backportable way for that before I hit the wall... Thanks again for the help! And you for your feedbacks. Regards.
Re: stop copying footers to r-headers_in?
On Fri, Oct 25, 2013 at 9:12 AM, Yann Ylavic ylavic@gmail.com wrote: On Thu, Oct 24, 2013 at 7:42 PM, Eric Covener cove...@gmail.com wrote: On Wed, Oct 23, 2013 at 8:04 AM, Yann Ylavic ylavic@gmail.com wrote: 1) add r-footers_in and use it in 2.2 and up by default Do that mean no API/ABI change ? In the sense that it needs to be backportable, yes -- but it will mean a behavior change to existing APIs. Depends on how you interpret it I guess, but I think the confusion over trailers/headers is something that needs to be forcible corrected in those service releases. Concretely, is request_rec::trailers_in/out (at the very end of the struct) something backportable or should the footers be stored elsewhere? yes, that's fine. According to [my understanding of] the rfc2616 and/or draft-ietf-httpbis-p1-messaging-24 about the HTTP trailer (chunking, TE and Trailer sections), they are hop-by-hop (anyway negociable/negociated hop-by-hop). They could then be stored in something more related to the request's connection(s), for *example* in the http_ctx_t used by ap_http_filter for input, which could probably be shared with protocol output filters too for the trailers_out. There is still the need to access them from r, for *example* (still) with something like the request_config of the http_module. But this looks like a big hack compared to the (simple) r-trailers_in/out... So, is the backportability something to care about for trunk (now) or things should be kept [as] simple [as possible] there and complications arise when backporting (using a different storage/access for example)? Generally would consider it as it goes into trunk, and jump through some level of hoops to keep them common when possible. My plans were to use request_rec for now and change this later if it oughts to, but it could be useful to point/suggest me a backportable way for that before I hit the wall... +1 -- Eric Covener cove...@gmail.com
Re: stop copying footers to r-headers_in?
On Wed, Oct 23, 2013 at 8:04 AM, Yann Ylavic ylavic@gmail.com wrote: Should I continue this way? Simply to propose patches? Sorry for not following up earlier. The patches are very much appreciated. But bite-sized ones are much easier for others to review. This might often mean fixing the rough corners of existing code in 1 pass, refactoring with no behavior change in another pass, then finally putting in the new stuff (just as a high level example). 1) add r-footers_in and use it in 2.2 and up by default Do that mean no API/ABI change ? In the sense that it needs to be backportable, yes -- but it will mean a behavior change to existing APIs. Depends on how you interpret it I guess, but I think the confusion over trailers/headers is something that needs to be forcible corrected in those service releases. Thanks again for the help!
Re: stop copying footers to r-headers_in?
Should I continue this way? Simply to propose patches?
On Sat, Oct 19, 2013 at 4:13 PM, Eric Covener cove...@gmail.com wrote:
Currently, when the body is consumed by a handler, a side effect is
reading footers that might be present and copying them to
r-headers_in.
This presents a series of problems.
* things that inspect r-headers_in expect it to be fluffed up much
earlier than midway through the handler phase
* if the handler looks at headers before reading the body, they could
differ from what's logged
* if the handler looks at headers after reading the body, mod_headers
was out of the loop if configured.
I am thinking:
now:
1) add r-footers_in and use it in 2.2 and up by default
Do that mean no API/ABI change ?
2) add a directive to copy them up to r-headers_in (for those broken
by the change)
soon:
3) add a hook to parse footers
later:
4) try to teach mod_headers to do something useful with that hook, but
not with existing directives.
5) teach mod_log_config to log from footers_in
Not done (yet), maybe I could try or am I completely bogged down with the
design?
Maybe it worth also :
6) teach ap_expr/ssl_lookup to interpolate from footers
7) teach mod_rewrite (hooked) how to play with footers
8) teach mod_proxy_http to forward the footers
A proposition for 8) follows.
Regards.
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c(revision 1534970)
+++ modules/proxy/proxy_util.c(working copy)
@@ -3114,11 +3114,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
char **old_te_val)
{
conn_rec *c = r-connection;
-int counter;
char *buf;
-const apr_array_header_t *headers_in_array;
-const apr_table_entry_t *headers_in;
-apr_table_t *headers_in_copy;
apr_bucket *e;
int do_100_continue;
conn_rec *origin = p_conn-connection;
@@ -3278,19 +3274,42 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
}
proxy_run_fixups(r);
+return ap_proxy_fill_hdrbrgd(p, header_brigade, r-headers_in, r,
+ old_cl_val, old_te_val);
+}
+
+PROXY_DECLARE(int) ap_proxy_fill_hdrbrgd(apr_pool_t *p,
+ apr_bucket_brigade
*header_brigade,
+ const apr_table_t *header_table,
+ request_rec *r, char **old_cl_val,
+ char **old_te_val)
+{
+const apr_array_header_t *headers_in_array;
+const apr_table_entry_t *headers_in;
+apr_table_t *headers_in_copy;
+apr_bucket *e;
+int counter;
+char *buf;
+
+/* easy path first */
+if (apr_is_empty_table(header_table)) {
+return OK;
+}
+
/*
- * Make a copy of the headers_in table before clearing the connection
+ * Make a copy of the header_table before clearing the connection
* headers as we need the connection headers later in the http output
* filter to prepare the correct response headers.
*
* Note: We need to take r-pool for apr_table_copy as the key / value
- * pairs in r-headers_in have been created out of r-pool and
- * p might be (and actually is) a longer living pool.
+ * pairs in header_table (eg. r-headers/footers_in) have been created
+ * out of r-pool and p might be (and actually is) a longer living
pool.
* This would trigger the bad pool ancestry abort in apr_table_copy if
* apr is compiled with APR_POOL_DEBUG.
*/
-headers_in_copy = apr_table_copy(r-pool, r-headers_in);
+headers_in_copy = apr_table_copy(r-pool, header_table);
ap_proxy_clear_connection(r, headers_in_copy);
+
/* send request headers */
headers_in_array = apr_table_elts(headers_in_copy);
headers_in = (const apr_table_entry_t *) headers_in_array-elts;
@@ -3317,7 +3336,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
* If we have used it then MAYBE: RFC2616 says we MAY propagate it.
* So let's make it configurable by env.
*/
-if (!strcasecmp(headers_in[counter].key,Proxy-Authorization)) {
+if (!strcasecmp(headers_in[counter].key, Proxy-Authorization)) {
if (r-user != NULL) { /* we've authenticated */
if (!apr_table_get(r-subprocess_env, Proxy-Chain-Auth))
{
continue;
@@ -3328,17 +3347,21 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
/* Skip Transfer-Encoding and Content-Length for now.
*/
if (!strcasecmp(headers_in[counter].key, Transfer-Encoding)) {
-*old_te_val = headers_in[counter].val;
+if (old_te_val) {
+*old_te_val = headers_in[counter].val;
+}
continue;
}
if (!strcasecmp(headers_in[counter].key, Content-Length)) {
-*old_cl_val =
Re: stop copying footers to r-headers_in?
On Sat, Oct 19, 2013 at 10:13:21AM -0400, Eric Covener wrote: now: 1) add r-footers_in and use it in 2.2 and up by default 2) add a directive to copy them up to r-headers_in (for those broken by the change) Bikeshed... r-trailers_in? +1 to all this anyway. I'd be tempted to (lazily) stop here to see whether any users actually care about trailers. It looks like a pain to thread this change through both versions of ap_get_mime_headers*().
RE: stop copying footers to r-headers_in?
-Original Message- From: Joe Orton Sent: Montag, 21. Oktober 2013 17:09 To: dev@httpd.apache.org Subject: Re: stop copying footers to r-headers_in? On Sat, Oct 19, 2013 at 10:13:21AM -0400, Eric Covener wrote: now: 1) add r-footers_in and use it in 2.2 and up by default 2) add a directive to copy them up to r-headers_in (for those broken by the change) Bikeshed... r-trailers_in? +1 to all this anyway. I'd be tempted to (lazily) stop here to see whether any users actually care about trailers. It looks like a pain to thread this change through both versions of ap_get_mime_headers*(). +1 to this laziness. Regards Rüdiger
Re: stop copying footers to r-headers_in?
On 21 Oct 2013, at 5:20 PM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: Bikeshed... r-trailers_in? +1 to all this anyway. I'd be tempted to (lazily) stop here to see whether any users actually care about trailers. It looks like a pain to thread this change through both versions of ap_get_mime_headers*(). +1 to this laziness. +1 to the laziness, +1 to r-trailers_in. Regards, Graham --
Re: stop copying footers to r-headers_in?
On Oct 21, 2013, at 11:37 AM, Graham Leggett minf...@sharp.fm wrote: On 21 Oct 2013, at 5:20 PM, Plüm, Rüdiger, Vodafone Group ruediger.pl...@vodafone.com wrote: Bikeshed... r-trailers_in? +1 to all this anyway. I'd be tempted to (lazily) stop here to see whether any users actually care about trailers. It looks like a pain to thread this change through both versions of ap_get_mime_headers*(). +1 to this laziness. +1 to the laziness, +1 to r-trailers_in. +1
Re: stop copying footers to r-headers_in?
Sounds like a good plan. Regards Rüdiger Eric Covener wrote: Currently, when the body is consumed by a handler, a side effect is reading footers that might be present and copying them to r-headers_in. This presents a series of problems. * things that inspect r-headers_in expect it to be fluffed up much earlier than midway through the handler phase * if the handler looks at headers before reading the body, they could differ from what's logged * if the handler looks at headers after reading the body, mod_headers was out of the loop if configured. I am thinking: now: 1) add r-footers_in and use it in 2.2 and up by default 2) add a directive to copy them up to r-headers_in (for those broken by the change) soon: 3) add a hook to parse footers later: 4) try to teach mod_headers to do something useful with that hook, but not with existing directives. 5) teach mod_log_config to log from footers_in Thanks and credit to Martin Holst Swende for discovering and reporting the problem. Any thoughts or volunteers? Also eager to hear some direction from Roy on this one
