You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Eric Covener <co...@gmail.com> on 2013/10/19 16:13:21 UTC
stop copying footers to r->headers_in?
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
--
Eric Covener
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Ruediger Pluem <rp...@apache.org>.
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
>
Re: stop copying footers to r->headers_in?
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 21, 2013, at 11:37 AM, Graham Leggett <mi...@sharp.fm> wrote:
> On 21 Oct 2013, at 5:20 PM, Plüm, Rüdiger, Vodafone Group <ru...@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?
Posted by Graham Leggett <mi...@sharp.fm>.
On 21 Oct 2013, at 5:20 PM, Plüm, Rüdiger, Vodafone Group <ru...@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?
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----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?
Posted by Joe Orton <jo...@redhat.com>.
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?
Posted by Eric Covener <co...@gmail.com>.
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
<ru...@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:covener@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
covener@gmail.com
RE: stop copying footers to r->headers_in?
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
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:covener@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?
Posted by Eric Covener <co...@gmail.com>.
something odd in proxy path when backend has
#0 0x00007f16f51b92e1 in apr_table_clear (t=0x0) at tables/apr_tables.c:467
467 t->a.nelts = 0;
(gdb) where
#0 0x00007f16f51b92e1 in apr_table_clear (t=0x0) at tables/apr_tables.c:467
#1 0x0000000000483bfc in read_chunked_trailers (ctx=0x7f16ac0125c0,
f=0x7f16ac012378, b=0x7f16ac00eec8, merge=0) at http_filters.c:198
#2 0x000000000048493d in ap_http_filter (f=0x7f16ac012378,
b=0x7f16ac00eec8, mode=AP_MODE_READBYTES, block=APR_NONBLOCK_READ,
readbytes=8192) at http_filters.c:448
#3 0x0000000000439d8e in ap_get_brigade (next=0x7f16ac012378,
bb=0x7f16ac00eec8, mode=AP_MODE_READBYTES, block=APR_NONBLOCK_READ,
readbytes=8192) at util_filter.c:553
#4 0x00007f16edb39794 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 0x00007f16edb3aaaa 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 0x00007f16edf51858 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 0x00007f16edf4c650 in proxy_handler (r=0x7f16ac004980) at mod_proxy.c:1157
#8 0x0000000000463ad1 in ap_run_handler (r=0x7f16ac004980) at config.c:169
#9 0x0000000000464524 in ap_invoke_handler (r=0x7f16ac004980) at config.c:433
#10 0x00000000004827ba in ap_process_async_request (r=0x7f16ac004980)
at http_request.c:317
#11 0x000000000047ebbb in ap_process_http_async_connection
(c=0x7f16e40b3790) at http_core.c:143
#12 0x000000000047eda8 in ap_process_http_connection
(c=0x7f16e40b3790) at http_core.c:228
#13 0x0000000000471fc9 in ap_run_process_connection (c=0x7f16e40b3790)
at connection.c:41
#14 0x00007f16ecea8f2c in process_socket (thd=0x111da28,
p=0x7f16e40b3488, sock=0x7f16e40b3500, cs=0x7f16e40b3708,
my_child_num=0, my_thread_num=0) at event.c:1077
#15 0x00007f16eceabd98 in worker_thread (thd=0x111da28,
dummy=0x7f16e40008c0) at event.c:2141
#16 0x00007f16f51d1f2a in dummy_worker (opaque=0x111da28) at
threadproc/unix/thread.c:142
#17 0x00007f16f4f8c182 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 <ch...@gmail.com> wrote:
> Patch for trunk as well
>
>
> On Wed, Jun 25, 2014 at 1:20 PM, Edward Lu <ch...@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 <yl...@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
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jul 15, 2014 at 11:07 AM, Plüm, Rüdiger, Vodafone Group
<ru...@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
covener@gmail.com
RE: stop copying footers to r->headers_in?
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Original Message-----
> From: Eric Covener [mailto:covener@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
> <ru...@vodafone.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Eric Covener [mailto:covener@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
> >> <ru...@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=revision&revision=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?
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jul 15, 2014 at 10:56 AM, Plüm, Rüdiger, Vodafone Group
<ru...@vodafone.com> wrote:
>
>
>> -----Original Message-----
>> From: Eric Covener [mailto:covener@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
>> <ru...@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=revision&revision=r1610707
RE: stop copying footers to r->headers_in?
Posted by "Houser, Rick" <ri...@us.pgds.com>.
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 <ri...@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:
>
> _<char1><char2> (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.pluem@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:covener@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
> > <ru...@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?
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Original Message-----
> From: Eric Covener [mailto:covener@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
> <ru...@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?
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jul 15, 2014 at 10:38 AM, Plüm, Rüdiger, Vodafone Group
<ru...@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
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jul 15, 2014, at 10:38 AM, Plüm, Rüdiger, Vodafone Group <ru...@vodafone.com> wrote:
>
>
>> -----Original Message-----
>> From: Eric Covener [mailto:covener@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?
Posted by Tim Bannister <is...@jellybaby.net>.
On 15 Jul 2014, at 15:38, Rüdiger Plüm <ru...@vodafone.com> wrote:
> -----Original Message-----
>> From: Eric Covener [mailto:covener@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 – isoma@jellybaby.net
RE: stop copying footers to r->headers_in?
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Original Message-----
> From: Eric Covener [mailto:covener@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?
Posted by Eric Covener <co...@gmail.com>.
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 <ch...@gmail.com> wrote:
> Patch for trunk as well
>
>
> On Wed, Jun 25, 2014 at 1:20 PM, Edward Lu <ch...@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 <yl...@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
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Edward Lu <ch...@gmail.com>.
Patch for trunk as well
On Wed, Jun 25, 2014 at 1:20 PM, Edward Lu <ch...@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 <yl...@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.
>>
>
>
Re: stop copying footers to r->headers_in?
Posted by Edward Lu <ch...@gmail.com>.
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 <yl...@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.
>
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
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?
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, May 7, 2014 at 11:47 AM, Joe Orton <jo...@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;
- *s = apr_palloc(r->pool, current_alloc);
+ *s = apr_palloc(r->pool, current_alloc + 1);
}
else if (bytes_handled + len > current_alloc) {
/* Increase the buffer size */
@@ -298,7 +325,7 @@
new_size = (bytes_handled + len) * 2;
}
- new_buffer = apr_palloc(r->pool, new_size);
+ new_buffer = apr_palloc(r->pool, new_size + 1);
/* Copy what we already had. */
memcpy(new_buffer, *s, bytes_handled);
@@ -327,8 +354,9 @@
if (last_char > *s && last_char[-1] == APR_ASCII_CR) {
last_char--;
}
+ bytes_handled = last_char - *s;
+ *read = bytes_handled;
*last_char = '\0';
- bytes_handled = last_char - *s;
/* If we're folding, we have more work to do.
*
@@ -344,8 +372,11 @@
apr_brigade_cleanup(bb);
/* We only care about the first byte. */
- rv = ap_get_brigade(r->proto_input_filters, bb,
AP_MODE_SPECULATIVE,
- APR_BLOCK_READ, 1);
+ rv = ap_get_brigade(f, bb, AP_MODE_SPECULATIVE, block, 1);
+ if (rv == APR_SUCCESS && block == APR_NONBLOCK_READ &&
+ APR_BRIGADE_EMPTY(bb)) {
+ rv = APR_EAGAIN;
+ }
if (rv != APR_SUCCESS) {
return rv;
}
@@ -401,38 +432,43 @@
}
next_size = n - bytes_handled;
-
- rv = ap_rgetline_core(&tmp, next_size,
- &next_len, r, 0, bb);
- if (rv != APR_SUCCESS) {
- return rv;
- }
-
- if (do_alloc && next_len > 0) {
+
+ 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;
- /* we need to alloc an extra byte for a null */
- new_buffer = apr_palloc(r->pool, new_size);
+ /* we need to alloc an extra byte for a nul */
+ new_buffer = apr_palloc(r->pool,
+ bytes_handled +
+ next_len + 1);
/* Copy what we already had. */
memcpy(new_buffer, *s, bytes_handled);
+ *s = new_buffer;
- /* copy the new line, including the trailing null */
- memcpy(new_buffer + bytes_handled, tmp, next_len + 1);
- *s = new_buffer;
+ /* copy the new line, including trailing nul */
+ last_char = *s + bytes_handled;
+ memcpy(last_char, tmp, next_len + 1);
}
-
last_char += next_len;
bytes_handled += next_len;
+ *read = bytes_handled;
}
+ if (rv != APR_SUCCESS) {
+ return rv;
}
+ }
+ }
else { /* next character is not tab or space */
break;
}
}
}
- *read = bytes_handled;
/* PR#43039: We shouldn't accept NULL bytes within the line */
if (strlen(*s) < bytes_handled) {
@@ -442,6 +478,13 @@
return APR_SUCCESS;
}
+AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
+ apr_size_t *read, request_rec *r,
+ int fold, apr_bucket_brigade *bb)
+{
+ return ap_rgetline_ex(s, n, read, r, fold, bb, APR_BLOCK_READ, NULL);
+}
+
#if APR_CHARSET_EBCDIC
AP_DECLARE(apr_status_t) ap_rgetline(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
Re: stop copying footers to r->headers_in?
Posted by Joe Orton <jo...@redhat.com>.
On Tue, May 06, 2014 at 07:45:42PM -0400, Eric Covener wrote:
> On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic <yl...@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
Re: stop copying footers to r->headers_in?
Posted by Jim Jagielski <ji...@jaguNET.com>.
On May 6, 2014, at 7:45 PM, Eric Covener <co...@gmail.com> wrote:
> On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Tue, May 6, 2014 at 1:41 PM, Eric Covener <co...@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
> covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, May 12, 2014 at 7:03 PM, Edward Lu <ch...@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?
Posted by Edward Lu <ch...@gmail.com>.
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 <yl...@gmail.com> wrote:
> On Sun, May 11, 2014 at 6:35 PM, Yann Ylavic <yl...@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?
Posted by Yann Ylavic <yl...@gmail.com>.
On Sun, May 11, 2014 at 6:35 PM, Yann Ylavic <yl...@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?
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, May 9, 2014 at 5:01 PM, Edward Lu <ch...@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,
+ saved_headers_in,
+ f->r->headers_in);
+ apr_table_clear(f->r->trailers_in);
+ }
+ if (saved_error_notes) {
+ apr_table_setn(f->r->notes, "error-notes",
+ saved_error_notes);
+ }
+ else if (error_notes) {
+ apr_table_unset(f->r->notes, "error-notes");
+ }
+ f->r->status = saved_status;
+
+ return rv;
}
}
}
[END]
Regards,
Yann.
Re: stop copying footers to r->headers_in?
Posted by Edward Lu <ch...@gmail.com>.
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 <ch...@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 <co...@gmail.com> wrote:
>
>> On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> > On Tue, May 6, 2014 at 1:41 PM, Eric Covener <co...@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
>> covener@gmail.com
>>
>
>
Re: stop copying footers to r->headers_in?
Posted by Edward Lu <ch...@gmail.com>.
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 <co...@gmail.com> wrote:
> On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic <yl...@gmail.com> wrote:
> > On Tue, May 6, 2014 at 1:41 PM, Eric Covener <co...@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
> covener@gmail.com
>
Re: stop copying footers to r->headers_in?
Posted by Eric Covener <co...@gmail.com>.
On Wed, May 7, 2014 at 6:35 AM, Ruediger Pluem <rp...@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
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Ruediger Pluem <rp...@apache.org>.
Eric Covener wrote:
> On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Tue, May 6, 2014 at 1:41 PM, Eric Covener <co...@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?
Posted by Eric Covener <co...@gmail.com>.
On Tue, May 6, 2014 at 6:44 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Tue, May 6, 2014 at 1:41 PM, Eric Covener <co...@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
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, May 6, 2014 at 1:41 PM, Eric Covener <co...@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?
Re: stop copying footers to r->headers_in?
Posted by Eric Covener <co...@gmail.com>.
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?
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Apr 1, 2014 at 11:35 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Tue, Apr 1, 2014 at 8:36 PM, Eric Covener <co...@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.
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Apr 1, 2014 at 8:36 PM, Eric Covener <co...@gmail.com> wrote:
>
> On Tue, Apr 1, 2014 at 2:00 PM, Yann Ylavic <yl...@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?
Posted by Eric Covener <co...@gmail.com>.
On Tue, Apr 1, 2014 at 2:00 PM, Yann Ylavic <yl...@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
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Apr 1, 2014 at 8:00 PM, Yann Ylavic <yl...@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?
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Apr 1, 2014 at 8:00 PM, Yann Ylavic <yl...@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?
Posted by Yann Ylavic <yl...@gmail.com>.
Hi Mike,
On Tue, Apr 1, 2014 at 9:21 PM, Mike Rumph <mi...@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?
Posted by Mike Rumph <mi...@oracle.com>.
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?
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Apr 1, 2014 at 4:53 PM, Yann Ylavic <yl...@gmail.com> wrote:
> 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.
>
Here is the corresponding patch.
For the time being the trailers are forwarded by mod_proxy
unconditionally, I need to take a closer at the RFC (server and proxy
cases).
The trailer hook is not used by anything, that where (at least)
mod_headers should go.
The trailers are not mergeable in r->header_in/out either (as said
above), but is that desirable?
Thanks for your feedbacks.
Index: server/config.c
===================================================================
--- server/config.c (revision 1583714)
+++ server/config.c (working copy)
@@ -71,6 +71,7 @@ AP_DECLARE_DATA ap_directive_t *ap_conftree = NULL
APR_HOOK_STRUCT(
APR_HOOK_LINK(header_parser)
+ APR_HOOK_LINK(trailer_parser)
APR_HOOK_LINK(pre_config)
APR_HOOK_LINK(check_config)
APR_HOOK_LINK(post_config)
@@ -85,6 +86,8 @@ APR_HOOK_STRUCT(
AP_IMPLEMENT_HOOK_RUN_ALL(int, header_parser,
(request_rec *r), (r), OK, DECLINED)
+AP_IMPLEMENT_HOOK_RUN_ALL(int, trailer_parser,
+ (request_rec *r), (r), OK, DECLINED)
AP_IMPLEMENT_HOOK_RUN_ALL(int, pre_config,
(apr_pool_t *pconf, apr_pool_t *plog,
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1583714)
+++ server/protocol.c (working copy)
@@ -193,12 +193,13 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
* 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.
+ * 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,9 +211,10 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
* 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,
- apr_size_t *read, request_rec *r,
- int fold, apr_bucket_brigade *bb)
+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;
@@ -220,6 +222,10 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
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 +236,28 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
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 +313,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
/* We'll assume the common case where one bucket is enough. */
if (!*s) {
current_alloc = len;
- *s = apr_palloc(r->pool, current_alloc);
+ *s = apr_palloc(r->pool, current_alloc + 1);
}
else if (bytes_handled + len > current_alloc) {
/* Increase the buffer size */
@@ -298,7 +324,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
new_size = (bytes_handled + len) * 2;
}
- new_buffer = apr_palloc(r->pool, new_size);
+ new_buffer = apr_palloc(r->pool, new_size + 1);
/* Copy what we already had. */
memcpy(new_buffer, *s, bytes_handled);
@@ -329,6 +355,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
}
*last_char = '\0';
bytes_handled = last_char - *s;
+ *read = bytes_handled;
/* If we're folding, we have more work to do.
*
@@ -344,8 +371,14 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
apr_brigade_cleanup(bb);
/* We only care about the first byte. */
- rv = ap_get_brigade(r->proto_input_filters, bb,
AP_MODE_SPECULATIVE,
- APR_BLOCK_READ, 1);
+ rv = ap_get_brigade(f, bb, AP_MODE_SPECULATIVE, block, 1);
+ if (block == APR_NONBLOCK_READ
+ && (APR_STATUS_IS_EAGAIN(rv)
+ || (rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb)))) {
+ if (rv == APR_SUCCESS) {
+ rv = APR_EAGAIN;
+ }
+ }
if (rv != APR_SUCCESS) {
return rv;
}
@@ -402,29 +435,36 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
next_size = n - bytes_handled;
- rv = ap_rgetline_core(&tmp, next_size,
- &next_len, r, 0, bb);
- if (rv != APR_SUCCESS) {
- return 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 && next_len > 0) {
- char *new_buffer;
- apr_size_t new_size = bytes_handled + next_len + 1;
+ if (do_alloc) {
+ char *new_buffer;
+ apr_size_t new_size = bytes_handled + next_len + 1;
- /* we need to alloc an extra byte for a null */
- new_buffer = apr_palloc(r->pool, new_size);
+ /* we need to alloc an extra byte for a nul */
+ new_buffer = apr_palloc(r->pool, new_size);
- /* Copy what we already had. */
- memcpy(new_buffer, *s, bytes_handled);
+ /* Copy what we already had. */
+ memcpy(new_buffer, *s, bytes_handled);
- /* copy the new line, including the trailing null */
- memcpy(new_buffer + bytes_handled, tmp, next_len + 1);
- *s = new_buffer;
+ /* copy the new line, including the trailing nul */
+ last_char = new_buffer + bytes_handled;
+ memcpy(last_char, tmp, next_len + 1);
+ *s = new_buffer;
+ }
+
+ last_char += next_len;
+ bytes_handled += next_len;
+ *read = bytes_handled;
}
-
- last_char += next_len;
- bytes_handled += next_len;
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
}
}
else { /* next character is not tab or space */
@@ -432,7 +472,6 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
}
}
}
- *read = bytes_handled;
/* PR#43039: We shouldn't accept NULL bytes within the line */
if (strlen(*s) < bytes_handled) {
@@ -442,6 +481,13 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
return APR_SUCCESS;
}
+AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
+ apr_size_t *read, request_rec *r,
+ int fold, apr_bucket_brigade *bb)
+{
+ return ap_rgetline_ex(s, n, read, r, fold, bb, APR_BLOCK_READ, NULL);
+}
+
#if APR_CHARSET_EBCDIC
AP_DECLARE(apr_status_t) ap_rgetline(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
@@ -755,18 +801,40 @@ static int field_name_len(const char *field)
return end - field;
}
-AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
apr_bucket_brigade *bb)
+struct ap_mime_state_t {
+ int fields_read;
+ char *last_field;
+ apr_size_t last_len;
+ apr_size_t alloc_len;
+ unsigned int fold :1,
+ done :1;
+};
+
+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);
+ }
+ else if (!(s = *state)) {
+ *state = s = apr_pcalloc(r->pool, sizeof(ap_mime_state_t));
+ }
+ else if (s->done) {
+ return APR_EOF;
+ }
+
/*
* Read header lines until we get the empty separator line, a read error,
* the connection closes (EOF), reach the server limit, or we timeout.
@@ -776,18 +844,26 @@ static int field_name_len(const char *field)
int folded = 0;
field = NULL;
- rv = ap_rgetline(&field, r->server->limit_req_fieldsize + 2,
- &len, r, 0, bb);
+ rv = ap_rgetline_ex(&field, r->server->limit_req_fieldsize + 2, &len,
+ r, 0, bb, block, f);
- if (rv != APR_SUCCESS) {
- if (APR_STATUS_IS_TIMEUP(rv)) {
- r->status = HTTP_REQUEST_TIME_OUT;
+ if (block == APR_NONBLOCK_READ
+ && APR_STATUS_IS_EAGAIN(rv)) {
+ if (len == 0) {
+ return rv;
}
- else {
- r->status = HTTP_BAD_REQUEST;
+ }
+ else if (rv != APR_SUCCESS) {
+ if (status) {
+ if (APR_STATUS_IS_TIMEUP(rv)) {
+ *status = HTTP_REQUEST_TIME_OUT;
+ }
+ else {
+ *status = HTTP_BAD_REQUEST;
+ }
}
- /* ap_rgetline returns APR_ENOSPC if it fills up the buffer before
+ /* ap_rgetline* returns APR_ENOSPC if it fills up the buffer before
* finding the end-of-line. This is only going to happen if it
* exceeds the configured limit for a field size.
*/
@@ -802,95 +878,108 @@ static int field_name_len(const char *field)
field_escaped = field = "";
}
- apr_table_setn(r->notes, "error-notes",
- apr_psprintf(r->pool,
+ if (html_notes) {
+ *html_notes = apr_psprintf(r->pool,
"Size of a request header field "
"exceeds server limit.<br />\n"
"<pre>\n%.*s\n</pre>\n",
field_name_len(field_escaped),
- field_escaped));
+ field_escaped);
+ }
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00561)
"Request header exceeds LimitRequestFieldSize%s"
"%.*s",
*field ? ": " : "",
field_name_len(field), field);
}
- return;
+ return rv;
}
- if (last_field != NULL) {
- if ((len > 0) && ((*field == '\t') || *field == ' ')) {
+ if (s->last_field != NULL) {
+ if ((len > 0) && (*field == '\t' || *field == ' ' || s->fold)) {
/* This line is a continuation of the preceding line(s),
* so append it to the line that we've set aside.
* Note: this uses a power-of-two allocator to avoid
* doing O(n) allocs and using O(n^2) space for
* continuations that span many many lines.
*/
- apr_size_t fold_len = last_len + len + 1; /* trailing null */
+ apr_size_t fold_len = s->last_len + len + 1; /* trailing nul */
if (fold_len >= (apr_size_t)(r->server->limit_req_fieldsize)) {
- r->status = HTTP_BAD_REQUEST;
+ if (status) {
+ *status = HTTP_BAD_REQUEST;
+ }
/* report what we have accumulated so far before the
* overflow (last_field) as the field with the problem
*/
- apr_table_setn(r->notes, "error-notes",
- apr_psprintf(r->pool,
+ if (html_notes) {
+ *html_notes = apr_psprintf(r->pool,
"Size of a request
header field "
"after folding "
"exceeds server limit.<br />\n"
"<pre>\n%.*s\n</pre>\n",
- field_name_len(last_field),
-
ap_escape_html(r->pool, last_field)));
+ field_name_len(s->last_field),
+ ap_escape_html(r->pool,
+ s->last_field));
+ }
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00562)
"Request header exceeds
LimitRequestFieldSize "
"after folding: %.*s",
- field_name_len(last_field), last_field);
- return;
+ field_name_len(s->last_field),
+ s->last_field);
+ return APR_ENOSPC;
}
- if (fold_len > alloc_len) {
+ if (fold_len > s->alloc_len) {
char *fold_buf;
- alloc_len += alloc_len;
- if (fold_len > alloc_len) {
- alloc_len = fold_len;
+ s->alloc_len += s->alloc_len;
+ if (fold_len > s->alloc_len) {
+ s->alloc_len = fold_len;
}
- fold_buf = (char *)apr_palloc(r->pool, alloc_len);
- memcpy(fold_buf, last_field, last_len);
- last_field = fold_buf;
+ fold_buf = (char *)apr_palloc(r->pool, s->alloc_len);
+ memcpy(fold_buf, s->last_field, s->last_len);
+ s->last_field = fold_buf;
}
- memcpy(last_field + last_len, field, len +1); /* +1 for nul */
- last_len += len;
+ memcpy(s->last_field + s->last_len, field, len +1); /* +nul */
+ s->last_len += len;
+ s->fold = 0;
folded = 1;
}
else /* not a continuation line */ {
if (r->server->limit_req_fields
- && (++fields_read > r->server->limit_req_fields)) {
- r->status = HTTP_BAD_REQUEST;
- apr_table_setn(r->notes, "error-notes",
- "The number of request header fields "
- "exceeds this server's limit.");
+ && (++s->fields_read > r->server->limit_req_fields)) {
+ if (status) {
+ *status = HTTP_BAD_REQUEST;
+ }
+ if (html_notes) {
+ *html_notes = "The number of request header fields "
+ "exceeds this server's limit.";
+ }
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00563)
"Number of request headers exceeds "
"LimitRequestFields");
- return;
+ return APR_ENOSPC;
}
- if (!(value = strchr(last_field, ':'))) { /* Find ':' or */
- r->status = HTTP_BAD_REQUEST; /* abort bad request */
- apr_table_setn(r->notes, "error-notes",
- apr_psprintf(r->pool,
+ if (!(value = strchr(s->last_field, ':'))) { /* ':' or */
+ if (status) {
+ *status = HTTP_BAD_REQUEST; /* abort bad request */
+ }
+ if (html_notes) {
+ *html_notes = apr_psprintf(r->pool,
"Request header field is "
"missing ':' separator.<br />\n"
"<pre>\n%.*s</pre>\n",
(int)LOG_NAME_MAX_LEN,
ap_escape_html(r->pool,
- last_field)));
+ s->last_field));
+ }
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00564)
"Request header field is missing ':' "
"separator: %.*s", (int)LOG_NAME_MAX_LEN,
- last_field);
- return;
+ s->last_field);
+ return APR_EINVAL;
}
tmp_field = value - 1; /* last character of field-name */
@@ -902,13 +991,13 @@ static int field_name_len(const char *field)
}
/* Strip LWS after field-name: */
- while (tmp_field > last_field
+ while (tmp_field > s->last_field
&& (*tmp_field == ' ' || *tmp_field == '\t')) {
*tmp_field-- = '\0';
}
/* Strip LWS after field-value: */
- tmp_field = last_field + last_len - 1;
+ tmp_field = s->last_field + s->last_len - 1;
while (tmp_field > value
&& (*tmp_field == ' ' || *tmp_field == '\t')) {
*tmp_field-- = '\0';
@@ -917,38 +1006,40 @@ static int field_name_len(const char *field)
if (conf->http_conformance & AP_HTTP_CONFORMANCE_STRICT) {
int err = 0;
- if (*last_field == '\0') {
+ if (*s->last_field == '\0') {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02425)
"Empty request header field
name not allowed");
}
- else if (ap_has_cntrl(last_field)) {
+ else if (ap_has_cntrl(s->last_field)) {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02426)
"[HTTP strict] Request header
field name contains "
"control character: %.*s",
- (int)LOG_NAME_MAX_LEN, last_field);
+ (int)LOG_NAME_MAX_LEN, s->last_field);
}
else if (ap_has_cntrl(value)) {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02427)
"Request header field '%.*s' contains "
"control character",
(int)LOG_NAME_MAX_LEN,
- last_field);
+ s->last_field);
}
if (err && !(conf->http_conformance &
AP_HTTP_CONFORMANCE_LOGONLY)) {
- r->status = err;
- return;
+ if (status) {
+ *status = err;
+ }
+ return APR_EINVAL;
}
}
- apr_table_addn(r->headers_in, last_field, value);
+ apr_table_addn(t, s->last_field, value);
/* reset the alloc_len so that we'll allocate a new
* buffer if we have to do any more folding: we can't
* use the previous buffer because its contents are
- * now part of r->headers_in
+ * now part of *t
*/
- alloc_len = 0;
+ s->alloc_len = 0;
} /* end if current line is not a continuation starting with tab */
}
@@ -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;
+ }
}
/* Combine multiple message-header fields with the same
* field-name, following RFC 2616, 4.2.
*/
- apr_table_compress(r->headers_in, APR_OVERLAP_TABLES_MERGE);
+ apr_table_compress(t, APR_OVERLAP_TABLES_MERGE);
/* enforce LimitRequestFieldSize for merged headers */
- apr_table_do(table_do_fn_check_lengths, r, r->headers_in, NULL);
+ apr_table_do(table_do_fn_check_lengths, r, t, NULL);
+
+ return APR_SUCCESS;
}
+AP_DECLARE(apr_status_t) ap_get_mime_headers_ex(request_rec *r,
+ apr_bucket_brigade *bb,
+ apr_read_type_e block,
+ ap_filter_t *f, apr_table_t *t,
+ ap_mime_state_t **state)
+{
+ return parse_mime_headers(r, bb, block, f, t, NULL, NULL, state);
+}
+
+AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
+ apr_bucket_brigade *bb)
+{
+ int status = OK;
+ const char *notes = NULL;
+
+ (void)parse_mime_headers(r, bb, APR_BLOCK_READ, NULL, r->headers_in,
+ &status, ¬es, NULL);
+ if (status != OK) {
+ r->status = status;
+ }
+ if (notes) {
+ apr_table_setn(r->notes, "error-notes", notes);
+ }
+}
+
AP_DECLARE(void) ap_get_mime_headers(request_rec *r)
{
apr_bucket_brigade *tmp_bb;
@@ -1010,9 +1141,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 +1422,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 +1434,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 1583714)
+++ modules/http/http_filters.c (working copy)
@@ -66,6 +66,8 @@ typedef struct http_filter_ctx
apr_off_t limit_used;
apr_int32_t chunk_used;
apr_int16_t chunkbits;
+ ap_mime_state_t *mime_state;
+ apr_bucket_brigade *tmp_bb;
enum
{
BODY_NONE, /* streamed data */
@@ -395,14 +397,6 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
}
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;
- }
-
break;
}
case BODY_NONE:
@@ -485,20 +479,29 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
}
case BODY_CHUNK_TRAILER: {
- rv = ap_get_brigade(f->next, b, mode, block, readbytes);
-
- /* for timeout */
- if (block == APR_NONBLOCK_READ
- && ((rv == APR_SUCCESS && APR_BRIGADE_EMPTY(b))
- || (APR_STATUS_IS_EAGAIN(rv)))) {
- return APR_EAGAIN;
+ if (ctx->tmp_bb == NULL) {
+ ctx->tmp_bb = apr_brigade_create(f->r->pool,
+ f->c->bucket_alloc);
}
-
+ rv = ap_get_mime_headers_ex(f->r, ctx->tmp_bb, block,
+ f->next, f->r->trailers_in,
+ &ctx->mime_state);
+ apr_brigade_cleanup(ctx->tmp_bb);
if (rv != APR_SUCCESS) {
return rv;
}
- break;
+ rv = ap_run_trailer_parser(f->r);
+ if (rv != OK && rv != DECLINED) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO()
+ "Invalid chunked trailer part");
+ return APR_EINVAL;
+ }
+
+ e = apr_bucket_eos_create(f->c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ ctx->eos_sent = 1;
+ return APR_SUCCESS;
}
default: {
break;
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1583714)
+++ modules/http/http_request.c (working copy)
@@ -463,6 +463,7 @@ static request_rec *internal_internal_redirect(con
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);
if (ap_is_HTTP_REDIRECT(new->status)) {
const char *location = apr_table_get(r->headers_out, "Location");
@@ -470,6 +471,8 @@ static request_rec *internal_internal_redirect(con
apr_table_setn(new->headers_out, "Location", location);
}
new->err_headers_out = r->err_headers_out;
+ new->trailers_out = r->trailers_out;
+
new->subprocess_env = rename_original_env(r->pool, r->subprocess_env);
new->notes = apr_table_make(r->pool, 5);
Index: modules/http/chunk_filter.c
===================================================================
--- modules/http/chunk_filter.c (revision 1583714)
+++ modules/http/chunk_filter.c (working copy)
@@ -25,6 +25,7 @@
#include "apr_want.h"
#include "httpd.h"
+#include "http_log.h"
#include "http_config.h"
#include "http_connection.h"
#include "http_core.h"
@@ -170,18 +171,47 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f,
* connection to the backend (mod_proxy) broke in the middle of the
* response. In order to signal the client that something went wrong
* we do not create the last-chunk marker and set c->keepalive to
- * AP_CONN_CLOSE in the core output filter.
+ * AP_CONN_CLOSE in the ap_http_outerror_filter() output filter.
*
* XXX: it would be nice to combine this with the end-of-chunk
* marker above, but this is a bit more straight-forward for
* now.
*/
if (eos && !f->ctx) {
- /* XXX: (2) trailers ... does not yet exist */
- e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF
- /* <trailers> */
- ASCII_CRLF, 5, c->bucket_alloc);
- APR_BUCKET_INSERT_BEFORE(eos, e);
+ /* TODO: make this configurable. */
+ if (apr_is_empty_table(f->r->trailers_out)) {
+ e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF
+ ASCII_CRLF, 5, c->bucket_alloc);
+ APR_BUCKET_INSERT_BEFORE(eos, e);
+ }
+ else {
+ const apr_array_header_t *trailers_elts;
+ const apr_table_entry_t *trailers;
+ int counter;
+
+ /* Last chunk */
+ e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF, 3,
+ c->bucket_alloc);
+ APR_BUCKET_INSERT_BEFORE(eos, e);
+
+ /* Trailer */
+ trailers_elts = apr_table_elts(f->r->trailers_out);
+ trailers = (const apr_table_entry_t *)trailers_elts->elts;
+ for (counter = 0; counter < trailers_elts->nelts; counter++) {
+ char *buf = apr_pstrcat(f->r->pool, trailers[counter].key,
+ ": ", trailers[counter].val,
+ ASCII_CRLF, NULL);
+ ap_xlate_proto_to_ascii(buf, strlen(buf));
+ e = apr_bucket_pool_create(buf, strlen(buf), f->r->pool,
+ c->bucket_alloc);
+ APR_BUCKET_INSERT_BEFORE(eos, e);
+ }
+
+ /* End of chunked */
+ e = apr_bucket_immortal_create(ASCII_CRLF, 2,
+ c->bucket_alloc);
+ APR_BUCKET_INSERT_BEFORE(eos, e);
+ }
}
/* pass the brigade to the next filter. */
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1583714)
+++ modules/proxy/proxy_util.c (working copy)
@@ -3199,11 +3199,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;
@@ -3363,19 +3359,44 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
}
proxy_run_fixups(r);
+
+ return ap_proxy_fill_hdrbrgd(p, header_brigade, r, c->bucket_alloc,
+ r->headers_in, old_cl_val, old_te_val);
+}
+
+PROXY_DECLARE(int) ap_proxy_fill_hdrbrgd(apr_pool_t *p,
+ apr_bucket_brigade *header_brigade,
+ request_rec *r,
+ apr_bucket_alloc_t *bucket_alloc,
+ const apr_table_t *headers_table,
+ char **old_cl_val, char **old_te_val)
+{
+ char *buf;
+ int counter;
+ const apr_array_header_t *headers_in_array;
+ const apr_table_entry_t *headers_in;
+ apr_table_t *headers_in_copy;
+ apr_bucket *e;
+
+ /* Easy path first. */
+ if (apr_is_empty_table(headers_table)) {
+ return OK;
+ }
+
/*
- * Make a copy of the headers_in table before clearing the connection
+ * Make a copy of the headers_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 headers_table (eg. r->headers/trailers_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, headers_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;
@@ -3413,11 +3434,17 @@ 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) {
+ /* Copy to avoid const away */
+ *old_te_val = apr_pstrdup(r->pool, headers_in[counter].val);
+ }
continue;
}
if (!strcasecmp(headers_in[counter].key, "Content-Length")) {
- *old_cl_val = headers_in[counter].val;
+ if (old_cl_val) {
+ /* Copy to avoid const away */
+ *old_cl_val = apr_pstrdup(r->pool, headers_in[counter].val);
+ }
continue;
}
@@ -3436,7 +3463,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
headers_in[counter].val, CRLF,
NULL);
ap_xlate_proto_to_ascii(buf, strlen(buf));
- e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc);
+ e = apr_bucket_pool_create(buf, strlen(buf), p, bucket_alloc);
APR_BRIGADE_INSERT_TAIL(header_brigade, e);
}
return OK;
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1583714)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -344,12 +344,27 @@ static int stream_reqbody_chunked(apr_pool_t *p,
bb = input_brigade;
}
- e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF
- /* <trailers> */
- ASCII_CRLF,
- 5, bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(bb, e);
+ /* Send the trailers if any. */
+ /* TODO: make this configurable. */
+ if (apr_is_empty_table(r->trailers_in)) {
+ e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF ASCII_CRLF,
+ 7, bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
+ }
+ else {
+ /* Last chunk */
+ e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF, 3, bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
+ /* Trailer */
+ ap_proxy_fill_hdrbrgd(p, bb, r, bucket_alloc, r->trailers_in,
NULL, NULL);
+
+
+ /* End of chunked */
+ e = apr_bucket_immortal_create(ASCII_CRLF, 2, bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
+ }
+
if (apr_table_get(r->subprocess_env, "proxy-sendextracrlf")) {
e = apr_bucket_immortal_create(ASCII_CRLF, 2, bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, e);
@@ -1011,9 +1026,11 @@ static request_rec *make_fake_req(conn_rec *c, req
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;
Index: modules/proxy/mod_proxy.h
===================================================================
--- modules/proxy/mod_proxy.h (revision 1583714)
+++ modules/proxy/mod_proxy.h (working copy)
@@ -980,6 +980,23 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
char **old_te_val);
/**
+ * Fill a HTTP request header brigade, old_cl_val and old_te_val as required.
+ * @param p pool
+ * @param header_brigade header brigade to fill
+ * @param r request
+ * @param bucket_alloc bucket allocator
+ * @param headers_table headers table to use
+ * @param old_cl_val stored old content-len val (if not NULL)
+ * @param old_te_val stored old TE val (if not NULL)
+ * @return OK
+ */
+PROXY_DECLARE(int) ap_proxy_fill_hdrbrgd(apr_pool_t *p,
+ apr_bucket_brigade *header_brigade,
+ request_rec *r,
+ apr_bucket_alloc_t *bucket_alloc,
+ const apr_table_t *headers_table,
+ char **old_cl_val, char **old_te_val);
+/**
* @param bucket_alloc bucket allocator
* @param r request
* @param p_conn proxy connection
Index: modules/generators/mod_info.c
===================================================================
--- modules/generators/mod_info.c (revision 1583714)
+++ modules/generators/mod_info.c (working copy)
@@ -336,6 +336,7 @@ static hook_lookup_t request_hooks[] = {
{"Transaction Logging", ap_hook_get_log_transaction},
{"Insert Errors", ap_hook_get_insert_error_filter},
{"Generate Log ID", ap_hook_get_generate_log_id},
+ {"Trailer Parse", ap_hook_get_trailer_parser},
{NULL},
};
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);
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,
Index: include/http_protocol.h
===================================================================
--- include/http_protocol.h (revision 1583714)
+++ include/http_protocol.h (working copy)
@@ -75,6 +75,28 @@ AP_DECLARE(void) ap_get_mime_headers(request_rec *
AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
apr_bucket_brigade *bb);
+typedef struct ap_mime_state_t ap_mime_state_t;
+/**
+ * Full version of ap_get_mime_headers() that requires a
+ * temporary brigade, a blocking mode, a destination table
+ * and an non-blocking (optional) context to work with
+ * @param r The current request
+ * @param bb temp brigade
+ * @param block Read blocking mode
+ * @param f The filter to read from (r->proto_input_filters if NULL)
+ * @param t The table to put the headers/trailers to
+ * @param state If not NULL, allows for reentrant (non-blocking) call,
+ * use *state = NULL on the first call to get an opaque
+ * record reusable in subsequent (reentrant) calls.
+ * @return APR_SUCCESS when done, APR_EAGAIN when non-blocking, or
+ * an unrecoverable APR error.
+ */
+AP_DECLARE(apr_status_t) ap_get_mime_headers_ex(request_rec *r,
+ apr_bucket_brigade *bb,
+ apr_read_type_e block,
+ ap_filter_t *f, apr_table_t *t,
+ ap_mime_state_t **state);
+
/* Finish up stuff after a request */
/**
@@ -632,6 +654,11 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
request_rec *r, int fold,
apr_bucket_brigade *bb);
+/** @see ap_rgetline */
+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);
/**
* Get the method number associated with the given string, assumed to
* contain an HTTP method. Returns M_INVALID if not recognized.
Index: include/http_config.h
===================================================================
--- include/http_config.h (revision 1583714)
+++ include/http_config.h (working copy)
@@ -1234,6 +1234,13 @@ AP_CORE_DECLARE(void *) ap_set_config_vectors(serv
AP_DECLARE_HOOK(int,header_parser,(request_rec *r))
/**
+ * Run the trailer parser functions for each module
+ * @param r The current request
+ * @return OK or DECLINED
+ */
+AP_DECLARE_HOOK(int,trailer_parser,(request_rec *r))
+
+/**
* Run the pre_config function for each module
* @param pconf The config pool
* @param plog The logging streams pool
Index: include/httpd.h
===================================================================
--- include/httpd.h (revision 1583714)
+++ include/httpd.h (working copy)
@@ -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;
};
/**
[EOS]
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
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 <co...@gmail.com> wrote:
> Resurrecting this thread. Yann, anything holding this one back?
>
>
> On Mon, Oct 28, 2013 at 9:43 AM, Yann Ylavic <yl...@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
> covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Eric Covener <co...@gmail.com>.
Resurrecting this thread. Yann, anything holding this one back?
On Mon, Oct 28, 2013 at 9:43 AM, Yann Ylavic <yl...@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
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
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?
Posted by Yann Ylavic <yl...@gmail.com>.
Regarding the "protocol" part, here is the latest patch.
This one renames the
ap_rgetline_ex() and ap_get_mime_headers_ex()
to respectively
ap_rgetline_from
() and
ap_get_mime_headers_from
() (more ap-ish name than apr-ish old ones, but it's just a proposition).
It also uses r->trailers_in/out instead of footers_in/out, this is how the
RFC calls them (again, it's just a proposition).
More importantly, ap_get_mime_headers_from() won't modify r->status nor
set r->notes now, while ap_get_mime_headers_core() will (still), thanks to
the new (static) ap_parse_mime_headers() used by both.
Moreover, the new hook :
+AP_IMPLEMENT_HOOK_RUN_ALL(int, trailer_parser,
+ (request_rec *r), (r), OK, DECLINED)
is introduced, though nothing is hooked there for now.
Hence ap_http_filter() does run these hooks after having parsed the trailer.
Should any hook return anything else than OK/DECLINED, ap_http_filter()
will also return an error (APR_EINVAL?), for the caller to know.
The goal is (maybe) to hook the HTTP protocol checker (really) first here
to stop/forbid the trailers if not compliant...
Thanks for your feedbacks,
regards.
Index: include/http_protocol.h
===================================================================
--- include/http_protocol.h (revision 1536297)
+++ include/http_protocol.h (working copy)
@@ -75,6 +75,27 @@ AP_DECLARE(void) ap_get_mime_headers(request_rec *
AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
apr_bucket_brigade *bb);
+typedef struct ap_mime_headers_ctx_t ap_mime_headers_ctx_t;
+/**
+ * Full version of ap_get_mime_headers() that requires a
+ * temporary brigade, a blocking mode, a destination table
+ * and an non-blocking (optional) context to work with
+ * @param r The current request
+ * @param bb temp brigade
+ * @param from The filter to read from (r->proto_input_filters if NULL)
+ * @param block Read blocking mode
+ * @param to The table to put the headers/trailers to
+ * @param ctx If not NULL, allows for reentrant non-blocking call
+ * @return APR_SUCCESS when done, APR_EAGAIN when non-blocking, or
+ * an unrecoverable APR error.
+ */
+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);
+
/* Finish up stuff after a request */
/**
@@ -632,6 +653,12 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
request_rec *r, int fold,
apr_bucket_brigade *bb);
+/** @see ap_rgetline */
+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);
/**
* Get the method number associated with the given string, assumed to
* contain an HTTP method. Returns M_INVALID if not recognized.
Index: include/http_config.h
===================================================================
--- include/http_config.h (revision 1536297)
+++ include/http_config.h (working copy)
@@ -1233,6 +1233,13 @@ AP_CORE_DECLARE(void *) ap_set_config_vectors(serv
AP_DECLARE_HOOK(int,header_parser,(request_rec *r))
/**
+ * Run the trailer parser functions for each module
+ * @param r The current request
+ * @return OK or DECLINED
+ */
+AP_DECLARE_HOOK(int,trailer_parser,(request_rec *r))
+
+/**
* Run the pre_config function for each module
* @param pconf The config pool
* @param plog The logging streams pool
Index: include/httpd.h
===================================================================
--- include/httpd.h (revision 1536297)
+++ include/httpd.h (working copy)
@@ -1037,6 +1037,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;
};
/**
Index: server/config.c
===================================================================
--- server/config.c (revision 1536297)
+++ server/config.c (working copy)
@@ -71,6 +71,7 @@ AP_DECLARE_DATA ap_directive_t *ap_conftree = NULL
APR_HOOK_STRUCT(
APR_HOOK_LINK(header_parser)
+ APR_HOOK_LINK(trailer_parser)
APR_HOOK_LINK(pre_config)
APR_HOOK_LINK(check_config)
APR_HOOK_LINK(post_config)
@@ -85,6 +86,8 @@ APR_HOOK_STRUCT(
AP_IMPLEMENT_HOOK_RUN_ALL(int, header_parser,
(request_rec *r), (r), OK, DECLINED)
+AP_IMPLEMENT_HOOK_RUN_ALL(int, trailer_parser,
+ (request_rec *r), (r), OK, DECLINED)
AP_IMPLEMENT_HOOK_RUN_ALL(int, pre_config,
(apr_pool_t *pconf, apr_pool_t *plog,
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1536297)
+++ server/protocol.c (working copy)
@@ -193,7 +193,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
* 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_from 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.
@@ -210,9 +210,11 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
* 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_from(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,
+ ap_filter_t *from,
+ apr_read_type_e block)
{
apr_status_t rv;
apr_bucket *e;
@@ -220,6 +222,10 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
char *pos, *last_char = *s;
int do_alloc = (*s == NULL), saw_eos = 0;
+ if (from == NULL) {
+ from = 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 +236,28 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
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(from, bb, AP_MODE_GETLINE, block, 0);
+ if (block == APR_NONBLOCK_READ
+ && ((rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb))
+ || APR_STATUS_IS_EAGAIN(rv))) {
+ *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 +313,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
/* We'll assume the common case where one bucket is
enough. */
if (!*s) {
current_alloc = len;
- *s = apr_palloc(r->pool, current_alloc);
+ *s = apr_palloc(r->pool, current_alloc + 1);
}
else if (bytes_handled + len > current_alloc) {
/* Increase the buffer size */
@@ -298,7 +324,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
new_size = (bytes_handled + len) * 2;
}
- new_buffer = apr_palloc(r->pool, new_size);
+ new_buffer = apr_palloc(r->pool, new_size + 1);
/* Copy what we already had. */
memcpy(new_buffer, *s, bytes_handled);
@@ -329,6 +355,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
}
*last_char = '\0';
bytes_handled = last_char - *s;
+ *read = bytes_handled;
/* If we're folding, we have more work to do.
*
@@ -344,8 +371,14 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
apr_brigade_cleanup(bb);
/* We only care about the first byte. */
- rv = ap_get_brigade(r->proto_input_filters, bb,
AP_MODE_SPECULATIVE,
- APR_BLOCK_READ, 1);
+ rv = ap_get_brigade(from, bb, AP_MODE_SPECULATIVE, block, 1);
+ if (block == APR_NONBLOCK_READ
+ && ((rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb))
+ || APR_STATUS_IS_EAGAIN(rv))) {
+ if (rv == APR_SUCCESS) {
+ rv = APR_EAGAIN;
+ }
+ }
if (rv != APR_SUCCESS) {
return rv;
}
@@ -402,13 +435,13 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
next_size = n - bytes_handled;
- rv = ap_rgetline_core(&tmp, next_size,
- &next_len, r, 0, bb);
- if (rv != APR_SUCCESS) {
- return rv;
- }
+ 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)))
+ && (next_len > 0)) {
- if (do_alloc && next_len > 0) {
+ if (do_alloc) {
char *new_buffer;
apr_size_t new_size = bytes_handled + next_len + 1;
@@ -419,12 +452,18 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
memcpy(new_buffer, *s, bytes_handled);
/* copy the new line, including the trailing null
*/
- memcpy(new_buffer + bytes_handled, tmp, next_len +
1);
+ last_char = new_buffer + bytes_handled;
+ memcpy(last_char, tmp, next_len + 1);
*s = new_buffer;
+ }
+
+ last_char += next_len;
+ bytes_handled += next_len;
+ *read = bytes_handled;
}
-
- last_char += next_len;
- bytes_handled += next_len;
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
}
}
else { /* next character is not tab or space */
@@ -432,7 +471,6 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
}
}
}
- *read = bytes_handled;
/* PR#43039: We shouldn't accept NULL bytes within the line */
if (strlen(*s) < bytes_handled) {
@@ -442,6 +480,13 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
return APR_SUCCESS;
}
+AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
+ apr_size_t *read, request_rec *r,
+ int fold, apr_bucket_brigade *bb)
+{
+ return ap_rgetline_from(s, n, read, r, fold, bb, NULL, APR_BLOCK_READ);
+}
+
#if APR_CHARSET_EBCDIC
AP_DECLARE(apr_status_t) ap_rgetline(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
@@ -755,18 +800,45 @@ static int field_name_len(const char *field)
return end - field;
}
-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;
+};
+
+static int ap_parse_mime_headers(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,
+ int *status, const char **html_notes)
{
- 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_headers_ctx_t thectx, *x;
core_server_config *conf =
ap_get_core_module_config(r->server->module_config);
+ if (status) {
+ *status = OK;
+ }
+ if (html_notes) {
+ *html_notes = NULL;
+ }
+
+ if (!ctx) {
+ x = &thectx;
+ memset(x, 0, sizeof *x);
+ }
+ else if (!(x = *ctx)) {
+ *ctx = x = apr_pcalloc(r->pool, sizeof(ap_mime_headers_ctx_t));
+ }
+ else if (x->done) {
+ return APR_EOF;
+ }
+
/*
* Read header lines until we get the empty separator line, a read
error,
* the connection closes (EOF), reach the server limit, or we timeout.
@@ -776,18 +848,25 @@ static int field_name_len(const char *field)
int folded = 0;
field = NULL;
- rv = ap_rgetline(&field, r->server->limit_req_fieldsize + 2,
- &len, r, 0, bb);
+ rv = ap_rgetline_from(&field, r->server->limit_req_fieldsize + 2,
&len,
+ r, 0, bb, from, block);
- if (rv != APR_SUCCESS) {
- if (APR_STATUS_IS_TIMEUP(rv)) {
- r->status = HTTP_REQUEST_TIME_OUT;
+ if (block == APR_NONBLOCK_READ && APR_STATUS_IS_EAGAIN(rv)) {
+ if (len == 0) {
+ return rv;
}
- else {
- r->status = HTTP_BAD_REQUEST;
+ }
+ else if (rv != APR_SUCCESS) {
+ if (status) {
+ if (APR_STATUS_IS_TIMEUP(rv)) {
+ *status = HTTP_REQUEST_TIME_OUT;
+ }
+ else {
+ *status = HTTP_BAD_REQUEST;
+ }
}
- /* ap_rgetline returns APR_ENOSPC if it fills up the buffer
before
+ /* ap_rgetline* returns APR_ENOSPC if it fills up the buffer
before
* finding the end-of-line. This is only going to happen if it
* exceeds the configured limit for a field size.
*/
@@ -802,95 +881,108 @@ static int field_name_len(const char *field)
field_escaped = field = "";
}
- apr_table_setn(r->notes, "error-notes",
- apr_psprintf(r->pool,
+ if (html_notes) {
+ *html_notes = apr_psprintf(r->pool,
"Size of a request header field
"
"exceeds server limit.<br />\n"
"<pre>\n%.*s\n</pre>\n",
field_name_len(field_escaped),
- field_escaped));
+ field_escaped);
+ }
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00561)
"Request header exceeds
LimitRequestFieldSize%s"
"%.*s",
*field ? ": " : "",
field_name_len(field), field);
}
- return;
+ return rv;
}
- if (last_field != NULL) {
- if ((len > 0) && ((*field == '\t') || *field == ' ')) {
+ if (x->last_field != NULL) {
+ if ((len > 0) && (*field == '\t' || *field == ' ' || x->fold))
{
/* This line is a continuation of the preceding line(s),
* so append it to the line that we've set aside.
* Note: this uses a power-of-two allocator to avoid
* doing O(n) allocs and using O(n^2) space for
* continuations that span many many lines.
*/
- apr_size_t fold_len = last_len + len + 1; /* trailing null
*/
+ apr_size_t fold_len = x->last_len + len + 1; /* trailing
nul */
if (fold_len >=
(apr_size_t)(r->server->limit_req_fieldsize)) {
- r->status = HTTP_BAD_REQUEST;
+ if (status) {
+ *status = HTTP_BAD_REQUEST;
+ }
/* report what we have accumulated so far before the
* overflow (last_field) as the field with the problem
*/
- apr_table_setn(r->notes, "error-notes",
- apr_psprintf(r->pool,
+ if (html_notes) {
+ *html_notes = apr_psprintf(r->pool,
"Size of a request header
field "
"after folding "
"exceeds server limit.<br
/>\n"
"<pre>\n%.*s\n</pre>\n",
- field_name_len(last_field),
- ap_escape_html(r->pool,
last_field)));
+
field_name_len(x->last_field),
+ ap_escape_html(r->pool,
+
x->last_field));
+ }
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(00562)
"Request header exceeds
LimitRequestFieldSize "
"after folding: %.*s",
- field_name_len(last_field), last_field);
- return;
+ field_name_len(x->last_field),
+ x->last_field);
+ return APR_ENOSPC;
}
- if (fold_len > alloc_len) {
+ if (fold_len > x->alloc_len) {
char *fold_buf;
- alloc_len += alloc_len;
- if (fold_len > alloc_len) {
- alloc_len = fold_len;
+ x->alloc_len += x->alloc_len;
+ if (fold_len > x->alloc_len) {
+ x->alloc_len = fold_len;
}
- fold_buf = (char *)apr_palloc(r->pool, alloc_len);
- memcpy(fold_buf, last_field, last_len);
- last_field = fold_buf;
+ fold_buf = (char *)apr_palloc(r->pool, x->alloc_len);
+ memcpy(fold_buf, x->last_field, x->last_len);
+ x->last_field = fold_buf;
}
- memcpy(last_field + last_len, field, len +1); /* +1 for
nul */
- last_len += len;
+ memcpy(x->last_field + x->last_len, field, len +1); /*
+nul */
+ x->last_len += len;
+ x->fold = 0;
folded = 1;
}
else /* not a continuation line */ {
if (r->server->limit_req_fields
- && (++fields_read > r->server->limit_req_fields)) {
- r->status = HTTP_BAD_REQUEST;
- apr_table_setn(r->notes, "error-notes",
- "The number of request header fields "
- "exceeds this server's limit.");
+ && (++x->fields_read >
r->server->limit_req_fields)) {
+ if (status) {
+ *status = HTTP_BAD_REQUEST;
+ }
+ if (html_notes) {
+ *html_notes = "The number of request header fields
"
+ "exceeds this server's limit.";
+ }
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(00563)
"Number of request headers exceeds "
"LimitRequestFields");
- return;
+ return APR_ENOSPC;
}
- if (!(value = strchr(last_field, ':'))) { /* Find ':'
or */
- r->status = HTTP_BAD_REQUEST; /* abort bad
request */
- apr_table_setn(r->notes, "error-notes",
- apr_psprintf(r->pool,
+ if (!(value = strchr(x->last_field, ':'))) { /* ':' or */
+ if (status) {
+ *status = HTTP_BAD_REQUEST; /* abort bad request */
+ }
+ if (html_notes) {
+ *html_notes = apr_psprintf(r->pool,
"Request header field is "
"missing ':' separator.<br
/>\n"
"<pre>\n%.*s</pre>\n",
(int)LOG_NAME_MAX_LEN,
ap_escape_html(r->pool,
-
last_field)));
+
x->last_field));
+ }
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(00564)
"Request header field is missing ':' "
"separator: %.*s", (int)LOG_NAME_MAX_LEN,
- last_field);
- return;
+ x->last_field);
+ return APR_EINVAL;
}
tmp_field = value - 1; /* last character of field-name */
@@ -902,13 +994,13 @@ static int field_name_len(const char *field)
}
/* Strip LWS after field-name: */
- while (tmp_field > last_field
+ while (tmp_field > x->last_field
&& (*tmp_field == ' ' || *tmp_field == '\t')) {
*tmp_field-- = '\0';
}
/* Strip LWS after field-value: */
- tmp_field = last_field + last_len - 1;
+ tmp_field = x->last_field + x->last_len - 1;
while (tmp_field > value
&& (*tmp_field == ' ' || *tmp_field == '\t')) {
*tmp_field-- = '\0';
@@ -917,66 +1009,103 @@ static int field_name_len(const char *field)
if (conf->http_conformance & AP_HTTP_CONFORMANCE_STRICT) {
int err = 0;
- if (*last_field == '\0') {
+ if (*x->last_field == '\0') {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02425)
"Empty request header field name not
allowed");
}
- else if (ap_has_cntrl(last_field)) {
+ else if (ap_has_cntrl(x->last_field)) {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02426)
"[HTTP strict] Request header field
name contains "
"control character: %.*s",
- (int)LOG_NAME_MAX_LEN, last_field);
+ (int)LOG_NAME_MAX_LEN,
x->last_field);
}
else if (ap_has_cntrl(value)) {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02427)
"Request header field '%.*s'
contains"
"control character",
(int)LOG_NAME_MAX_LEN,
- last_field);
+ x->last_field);
}
if (err && !(conf->http_conformance &
AP_HTTP_CONFORMANCE_LOGONLY)) {
- r->status = err;
- return;
+ if (status) {
+ *status = err;
+ }
+ return APR_EINVAL;
}
}
- apr_table_addn(r->headers_in, last_field, value);
+ apr_table_addn(to, x->last_field, value);
/* reset the alloc_len so that we'll allocate a new
* buffer if we have to do any more folding: we can't
* use the previous buffer because its contents are
- * now part of r->headers_in
+ * now part of *to
*/
- alloc_len = 0;
+ x->alloc_len = 0;
} /* end if current line is not a continuation starting with
tab */
}
- /* Found a blank line, stop. */
- if (len == 0) {
- break;
- }
-
/* Keep track of this line so that we can parse it on
* the next loop iteration. (In the folded case, last_field
* has been updated already.)
*/
if (!folded) {
- last_field = field;
- last_len = len;
+ x->last_field = field;
+ x->last_len = len;
}
+
+ /* We may still be EAGAIN here */
+ if (rv != APR_SUCCESS) {
+ AP_DEBUG_ASSERT(APR_STATUS_IS_EAGAIN(rv));
+ x->fold = 1;
+ return rv;
+ }
+
+ /* Found a blank line, stop. */
+ if (len == 0) {
+ x->done = 1;
+ break;
+ }
}
/* Combine multiple message-header fields with the same
* field-name, following RFC 2616, 4.2.
*/
- apr_table_compress(r->headers_in, APR_OVERLAP_TABLES_MERGE);
+ apr_table_compress(to, APR_OVERLAP_TABLES_MERGE);
/* enforce LimitRequestFieldSize for merged headers */
- apr_table_do(table_do_fn_check_lengths, r, r->headers_in, NULL);
+ apr_table_do(table_do_fn_check_lengths, r, to, NULL);
+
+ return APR_SUCCESS;
}
+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, NULL, APR_BLOCK_READ,
+ r->headers_in, ctx, NULL, NULL);
+}
+
+AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
+ apr_bucket_brigade *bb)
+{
+ int status = OK;
+ const char *notes = NULL;
+
+ (void)ap_parse_mime_headers(r, bb, NULL, APR_BLOCK_READ,
+ r->headers_in, NULL, &status, ¬es);
+ if (status != OK) {
+ r->status = status;
+ }
+ if (notes) {
+ apr_table_setn(r->notes, "error-notes", notes);
+ }
+}
+
AP_DECLARE(void) ap_get_mime_headers(request_rec *r)
{
apr_bucket_brigade *tmp_bb;
@@ -1010,9 +1139,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);
@@ -1280,6 +1411,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
@@ -1291,6 +1423,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 1536297)
+++ modules/http/http_filters.c (working copy)
@@ -66,6 +66,8 @@ typedef struct http_filter_ctx
apr_off_t limit_used;
apr_int32_t chunk_used;
apr_int16_t chunkbits;
+ ap_mime_headers_ctx_t *mime_hdrs_ctx;
+ apr_bucket_brigade *tmp_bb;
enum
{
BODY_NONE, /* streamed data */
@@ -384,17 +386,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
apr_bucket_delete(e);
e = APR_BRIGADE_FIRST(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;
- }
-
+ again = 1; /* come around again */
break;
}
case BODY_NONE:
@@ -471,20 +465,30 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
}
case BODY_CHUNK_TRAILER: {
- rv = ap_get_brigade(f->next, b, mode, block, readbytes);
-
- /* for timeout */
- if (block == APR_NONBLOCK_READ
- && ((rv == APR_SUCCESS && APR_BRIGADE_EMPTY(b))
- || (APR_STATUS_IS_EAGAIN(rv)))) {
- return APR_EAGAIN;
+ if (ctx->tmp_bb == NULL) {
+ ctx->tmp_bb = apr_brigade_create(f->r->pool,
+ f->c->bucket_alloc);
}
+ rv = ap_get_mime_headers_from(f->r, ctx->tmp_bb,
+ f->next, block,
f->r->trailers_in,
+ &ctx->mime_hdrs_ctx);
+ apr_brigade_cleanup(ctx->tmp_bb);
if (rv != APR_SUCCESS) {
return rv;
}
- break;
+ rv = ap_run_trailer_parser(f->r);
+ if (rv != OK && rv != DECLINED) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO()
+ "Invalid chunked trailer part");
+ return APR_EINVAL;
+ }
+
+ e = apr_bucket_eos_create(f->c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ ctx->eos_sent = 1;
+ return APR_SUCCESS;
}
default: {
break;
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1536297)
+++ modules/http/http_request.c (working copy)
@@ -463,6 +463,7 @@ static request_rec *internal_internal_redirect(con
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);
if (ap_is_HTTP_REDIRECT(new->status)) {
const char *location = apr_table_get(r->headers_out, "Location");
@@ -470,6 +471,8 @@ static request_rec *internal_internal_redirect(con
apr_table_setn(new->headers_out, "Location", location);
}
new->err_headers_out = r->err_headers_out;
+ new->trailers_out = r->trailers_out;
+
new->subprocess_env = rename_original_env(r->pool, r->subprocess_env);
new->notes = apr_table_make(r->pool, 5);
Index: modules/http/chunk_filter.c
===================================================================
--- modules/http/chunk_filter.c (revision 1536297)
+++ modules/http/chunk_filter.c (working copy)
@@ -25,6 +25,7 @@
#include "apr_want.h"
#include "httpd.h"
+#include "http_log.h"
#include "http_config.h"
#include "http_connection.h"
#include "http_core.h"
@@ -170,18 +171,56 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f,
* connection to the backend (mod_proxy) broke in the middle of the
* response. In order to signal the client that something went
wrong
* we do not create the last-chunk marker and set c->keepalive to
- * AP_CONN_CLOSE in the core output filter.
+ * AP_CONN_CLOSE in the ap_http_outerror_filter() output filter.
*
* XXX: it would be nice to combine this with the end-of-chunk
* marker above, but this is a bit more straight-forward for
* now.
*/
if (eos && !f->ctx) {
- /* XXX: (2) trailers ... does not yet exist */
+ if (!apr_is_empty_table(f->r->trailers_out)) {
+ const apr_array_header_t *trailers_elts;
+ const apr_table_entry_t *trailers;
+ int counter;
+
+ /* Prevent HTTP Response Splitting */
+ if (APR_BUCKET_NEXT(eos) != APR_BRIGADE_SENTINEL(b)) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO()
+ "Sent more bytes of response body than
expected");
+ return APR_EGENERAL;
+ }
+ APR_BUCKET_REMOVE(eos);
+
+ e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF, 3,
+ c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+
+ /* Put the trailers in the output brigade (code stolen from
+ * ap_proxy_fill_hdrbrgd).
+ */
+ trailers_elts = apr_table_elts(f->r->trailers_out);
+ trailers = (const apr_table_entry_t *)trailers_elts->elts;
+ for (counter = 0; counter < trailers_elts->nelts;
counter++) {
+ char *buf = apr_pstrcat(f->r->pool,
trailers[counter].key,
+ ": ", trailers[counter].val,
+ ASCII_CRLF, NULL);
+ ap_xlate_proto_to_ascii(buf, strlen(buf));
+ e = apr_bucket_pool_create(buf, strlen(buf),
f->r->pool,
+ c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ }
+
+ e = apr_bucket_immortal_create(ASCII_CRLF, 2,
+ c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+
+ APR_BRIGADE_INSERT_TAIL(b, eos);
+ }
+ else {
e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF
- /* <trailers> */
ASCII_CRLF, 5, c->bucket_alloc);
APR_BUCKET_INSERT_BEFORE(eos, e);
+ }
}
/* pass the brigade to the next filter. */
[EOS]
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Oct 28, 2013 at 2:23 AM, Eric Covener <co...@gmail.com> wrote:
> On Tue, Oct 22, 2013 at 2:18 PM, Yann Ylavic <yl...@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?
Posted by Eric Covener <co...@gmail.com>.
On Tue, Oct 22, 2013 at 2:18 PM, Yann Ylavic <yl...@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?
Posted by Yann Ylavic <yl...@gmail.com>.
Hi,
even if I don't really need the footers (don't know what to do with yet), I
need them to fully parsed in ap_http_filter(), in blocking and non-blocking
modes: so I'm diligent (and volonteer) for at least a part of the work...
The following patch introduces r->footers_in and the new ap_rgetline_ex()
and ap_get_mime_headers_ex() functions.
Their relation with the existing *_core() functions is as follow :
+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 *from);
AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
int fold, apr_bucket_brigade *bb)
{
return ap_rgetline_ex(s, n, read, r, fold, bb, APR_BLOCK_READ, NULL);
}
+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);
AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
apr_bucket_brigade *bb)
{
(void)ap_get_mime_headers_ex(r, bb, r->headers_in, NULL,
APR_BLOCK_READ, NULL);
}
The new ap_mime_headers_ctx_t struct aim to give ap_get_mime_headers_ex()
reentrance, with an (opaque) parsing context providable by the caller
(likely when non-blocking). The context is local (one shot) if pctx is NULL
or created if *pctx is NULL.
r->footers_in is created/copied wherever r->headers_in is, and filled in
ap_http_filter() in both blocking and non-blocking modes.
What this patch does not is merging r->footers_in and r->headers_in
whenever it should be, I need to have a closer look at the RFC on this
point.
Hope it can be useful.
Regards,
Yann.
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1534668)
+++ server/protocol.c (working copy)
@@ -193,7 +193,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
* 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.
@@ -210,9 +210,11 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
* 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,
- apr_size_t *read, request_rec *r,
- int fold, apr_bucket_brigade *bb)
+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 *from)
{
apr_status_t rv;
apr_bucket *e;
@@ -220,6 +222,10 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
char *pos, *last_char = *s;
int do_alloc = (*s == NULL), saw_eos = 0;
+ if (from == NULL) {
+ from = 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 +236,26 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
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(from, bb, AP_MODE_GETLINE, block, 0);
+ if (block == APR_NONBLOCK_READ
+ && ((rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb))
+ || APR_STATUS_IS_EAGAIN(rv))) {
+ *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';
+ }
+ return APR_EAGAIN;
+ }
if (rv != APR_SUCCESS) {
return rv;
}
@@ -287,7 +311,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
/* We'll assume the common case where one bucket is
enough. */
if (!*s) {
current_alloc = len;
- *s = apr_palloc(r->pool, current_alloc);
+ *s = apr_palloc(r->pool, current_alloc + 1);
}
else if (bytes_handled + len > current_alloc) {
/* Increase the buffer size */
@@ -298,7 +322,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
new_size = (bytes_handled + len) * 2;
}
- new_buffer = apr_palloc(r->pool, new_size);
+ new_buffer = apr_palloc(r->pool, new_size + 1);
/* Copy what we already had. */
memcpy(new_buffer, *s, bytes_handled);
@@ -329,6 +353,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
}
*last_char = '\0';
bytes_handled = last_char - *s;
+ *read = bytes_handled;
/* If we're folding, we have more work to do.
*
@@ -344,8 +369,12 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
apr_brigade_cleanup(bb);
/* We only care about the first byte. */
- rv = ap_get_brigade(r->proto_input_filters, bb,
AP_MODE_SPECULATIVE,
- APR_BLOCK_READ, 1);
+ rv = ap_get_brigade(from, bb, AP_MODE_SPECULATIVE, block, 1);
+ if (block == APR_NONBLOCK_READ
+ && ((rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb))
+ || APR_STATUS_IS_EAGAIN(rv))) {
+ return APR_EAGAIN;
+ }
if (rv != APR_SUCCESS) {
return rv;
}
@@ -402,13 +431,11 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
next_size = n - bytes_handled;
- rv = ap_rgetline_core(&tmp, next_size,
- &next_len, r, 0, bb);
- if (rv != APR_SUCCESS) {
- return rv;
- }
-
- if (do_alloc && next_len > 0) {
+ rv = ap_rgetline_ex(&tmp, next_size,
+ &next_len, r, 0, bb, block, from);
+ if (rv == APR_SUCCESS || (block == APR_NONBLOCK_READ &&
+ APR_STATUS_IS_EAGAIN(rv))) {
+ if (do_alloc && next_len > 0) {
char *new_buffer;
apr_size_t new_size = bytes_handled + next_len + 1;
@@ -419,12 +446,18 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
memcpy(new_buffer, *s, bytes_handled);
/* copy the new line, including the trailing null
*/
- memcpy(new_buffer + bytes_handled, tmp, next_len +
1);
+ last_char = new_buffer + bytes_handled;
+ memcpy(last_char, tmp, next_len + 1);
*s = new_buffer;
+ }
+
+ last_char += next_len;
+ bytes_handled += next_len;
+ *read = bytes_handled;
}
-
- last_char += next_len;
- bytes_handled += next_len;
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
}
}
else { /* next character is not tab or space */
@@ -432,7 +465,6 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
}
}
}
- *read = bytes_handled;
/* PR#43039: We shouldn't accept NULL bytes within the line */
if (strlen(*s) < bytes_handled) {
@@ -442,6 +474,13 @@ AP_DECLARE(apr_time_t) ap_rationalize_mtime(reques
return APR_SUCCESS;
}
+AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
+ apr_size_t *read, request_rec *r,
+ int fold, apr_bucket_brigade *bb)
+{
+ return ap_rgetline_ex(s, n, read, r, fold, bb, APR_BLOCK_READ, NULL);
+}
+
#if APR_CHARSET_EBCDIC
AP_DECLARE(apr_status_t) ap_rgetline(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
@@ -755,18 +794,38 @@ static int field_name_len(const char *field)
return end - field;
}
-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;
char *field;
char *value;
apr_size_t len;
- int fields_read = 0;
char *tmp_field;
+ ap_mime_headers_ctx_t thectx, *ctx;
core_server_config *conf =
ap_get_core_module_config(r->server->module_config);
+ if (!pctx) {
+ ctx = &thectx;
+ memset(ctx, 0, sizeof *ctx);
+ }
+ else if (!(ctx = *pctx)) {
+ *pctx = ctx = apr_pcalloc(r->pool, sizeof(ap_mime_headers_ctx_t));
+ }
+ else if (ctx->done) {
+ return APR_EOF;
+ }
+
/*
* Read header lines until we get the empty separator line, a read
error,
* the connection closes (EOF), reach the server limit, or we timeout.
@@ -776,10 +835,15 @@ static int field_name_len(const char *field)
int folded = 0;
field = NULL;
- rv = ap_rgetline(&field, r->server->limit_req_fieldsize + 2,
- &len, r, 0, bb);
+ rv = ap_rgetline_ex(&field, r->server->limit_req_fieldsize + 2,
+ &len, r, 0, bb, block, from);
- if (rv != APR_SUCCESS) {
+ if (block == APR_NONBLOCK_READ && APR_STATUS_IS_EAGAIN(rv)) {
+ if (len == 0) {
+ return rv;
+ }
+ }
+ else if (rv != APR_SUCCESS) {
if (APR_STATUS_IS_TIMEUP(rv)) {
r->status = HTTP_REQUEST_TIME_OUT;
}
@@ -787,7 +851,7 @@ static int field_name_len(const char *field)
r->status = HTTP_BAD_REQUEST;
}
- /* ap_rgetline returns APR_ENOSPC if it fills up the buffer
before
+ /* ap_rgetline* returns APR_ENOSPC if it fills up the buffer
before
* finding the end-of-line. This is only going to happen if it
* exceeds the configured limit for a field size.
*/
@@ -815,18 +879,18 @@ static int field_name_len(const char *field)
*field ? ": " : "",
field_name_len(field), field);
}
- return;
+ return rv;
}
- if (last_field != NULL) {
- if ((len > 0) && ((*field == '\t') || *field == ' ')) {
+ if (ctx->last_field != NULL) {
+ if ((len > 0) && (*field == '\t' || *field == ' ' ||
ctx->fold)) {
/* This line is a continuation of the preceding line(s),
* so append it to the line that we've set aside.
* Note: this uses a power-of-two allocator to avoid
* doing O(n) allocs and using O(n^2) space for
* continuations that span many many lines.
*/
- apr_size_t fold_len = last_len + len + 1; /* trailing null
*/
+ apr_size_t fold_len = ctx->last_len + len + 1; /* trailing
null */
if (fold_len >=
(apr_size_t)(r->server->limit_req_fieldsize)) {
r->status = HTTP_BAD_REQUEST;
@@ -839,33 +903,35 @@ static int field_name_len(const char *field)
"after folding "
"exceeds server limit.<br
/>\n"
"<pre>\n%.*s\n</pre>\n",
- field_name_len(last_field),
- ap_escape_html(r->pool,
last_field)));
+
field_name_len(ctx->last_field),
+ ap_escape_html(r->pool,
ctx->last_field)));
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(00562)
"Request header exceeds
LimitRequestFieldSize "
"after folding: %.*s",
- field_name_len(last_field), last_field);
- return;
+ field_name_len(ctx->last_field),
ctx->last_field);
+ return APR_ENOSPC;
}
- if (fold_len > alloc_len) {
+ if (fold_len > ctx->alloc_len) {
char *fold_buf;
- alloc_len += alloc_len;
- if (fold_len > alloc_len) {
- alloc_len = fold_len;
+ ctx->alloc_len += ctx->alloc_len;
+ if (fold_len > ctx->alloc_len) {
+ ctx->alloc_len = fold_len;
}
- fold_buf = (char *)apr_palloc(r->pool, alloc_len);
- memcpy(fold_buf, last_field, last_len);
- last_field = fold_buf;
+ fold_buf = (char *)apr_palloc(r->pool, ctx->alloc_len);
+ memcpy(fold_buf, ctx->last_field, ctx->last_len);
+ ctx->last_field = fold_buf;
}
- memcpy(last_field + last_len, field, len +1); /* +1 for
nul */
- last_len += len;
+ memcpy(ctx->last_field + ctx->last_len,
+ field, len +1); /* +1 for nul */
+ ctx->last_len += len;
+ ctx->fold = 0;
folded = 1;
}
else /* not a continuation line */ {
if (r->server->limit_req_fields
- && (++fields_read > r->server->limit_req_fields)) {
+ && (++ctx->fields_read > r->server->limit_req_fields))
{
r->status = HTTP_BAD_REQUEST;
apr_table_setn(r->notes, "error-notes",
"The number of request header fields "
@@ -873,10 +939,10 @@ static int field_name_len(const char *field)
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(00563)
"Number of request headers exceeds "
"LimitRequestFields");
- return;
+ return APR_ENOSPC;
}
- if (!(value = strchr(last_field, ':'))) { /* Find ':'
or */
+ if (!(value = strchr(ctx->last_field, ':'))) { /* Find ':'
or */
r->status = HTTP_BAD_REQUEST; /* abort bad
request */
apr_table_setn(r->notes, "error-notes",
apr_psprintf(r->pool,
@@ -885,12 +951,12 @@ static int field_name_len(const char *field)
"<pre>\n%.*s</pre>\n",
(int)LOG_NAME_MAX_LEN,
ap_escape_html(r->pool,
-
last_field)));
+
ctx->last_field)));
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(00564)
"Request header field is missing ':' "
"separator: %.*s", (int)LOG_NAME_MAX_LEN,
- last_field);
- return;
+ ctx->last_field);
+ return APR_EINVAL;
}
tmp_field = value - 1; /* last character of field-name */
@@ -902,13 +968,13 @@ static int field_name_len(const char *field)
}
/* Strip LWS after field-name: */
- while (tmp_field > last_field
+ while (tmp_field > ctx->last_field
&& (*tmp_field == ' ' || *tmp_field == '\t')) {
*tmp_field-- = '\0';
}
/* Strip LWS after field-value: */
- tmp_field = last_field + last_len - 1;
+ tmp_field = ctx->last_field + ctx->last_len - 1;
while (tmp_field > value
&& (*tmp_field == ' ' || *tmp_field == '\t')) {
*tmp_field-- = '\0';
@@ -917,66 +983,81 @@ static int field_name_len(const char *field)
if (conf->http_conformance & AP_HTTP_CONFORMANCE_STRICT) {
int err = 0;
- if (*last_field == '\0') {
+ if (*ctx->last_field == '\0') {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02425)
"Empty request header field name not
allowed");
}
- else if (ap_has_cntrl(last_field)) {
+ else if (ap_has_cntrl(ctx->last_field)) {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02426)
"[HTTP strict] Request header field
name contains "
"control character: %.*s",
- (int)LOG_NAME_MAX_LEN, last_field);
+ (int)LOG_NAME_MAX_LEN,
ctx->last_field);
}
else if (ap_has_cntrl(value)) {
err = HTTP_BAD_REQUEST;
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02427)
"Request header field '%.*s'
contains"
"control character",
(int)LOG_NAME_MAX_LEN,
- last_field);
+ ctx->last_field);
}
if (err && !(conf->http_conformance &
AP_HTTP_CONFORMANCE_LOGONLY)) {
r->status = err;
- return;
+ return APR_EINVAL;
}
}
- apr_table_addn(r->headers_in, last_field, value);
+ apr_table_addn(to, ctx->last_field, value);
/* reset the alloc_len so that we'll allocate a new
* buffer if we have to do any more folding: we can't
* use the previous buffer because its contents are
- * now part of r->headers_in
+ * now part of *to
*/
- alloc_len = 0;
+ ctx->alloc_len = 0;
} /* end if current line is not a continuation starting with
tab */
}
- /* Found a blank line, stop. */
- if (len == 0) {
- break;
- }
-
/* Keep track of this line so that we can parse it on
* the next loop iteration. (In the folded case, last_field
* has been updated already.)
*/
if (!folded) {
- last_field = field;
- last_len = len;
+ ctx->last_field = field;
+ ctx->last_len = len;
}
+
+ /* We may still be EAGAIN here */
+ if (rv != APR_SUCCESS) {
+ ctx->fold = 1;
+ return rv;
+ }
+
+ /* Found a blank line, stop. */
+ if (len == 0) {
+ ctx->done = 1;
+ break;
+ }
}
/* Combine multiple message-header fields with the same
* field-name, following RFC 2616, 4.2.
*/
- apr_table_compress(r->headers_in, APR_OVERLAP_TABLES_MERGE);
+ apr_table_compress(to, APR_OVERLAP_TABLES_MERGE);
/* enforce LimitRequestFieldSize for merged headers */
- apr_table_do(table_do_fn_check_lengths, r, r->headers_in, NULL);
+ apr_table_do(table_do_fn_check_lengths, r, to, NULL);
+
+ return APR_SUCCESS;
}
+AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
apr_bucket_brigade *bb)
+{
+ (void)ap_get_mime_headers_ex(r, bb, r->headers_in, NULL,
+ APR_BLOCK_READ, NULL);
+}
+
AP_DECLARE(void) ap_get_mime_headers(request_rec *r)
{
apr_bucket_brigade *tmp_bb;
@@ -1010,6 +1091,7 @@ 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->footers_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);
@@ -1280,6 +1362,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->footers_in = apr_table_copy(rnew->pool, r->footers_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
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1534668)
+++ modules/http/http_filters.c (working copy)
@@ -66,6 +66,8 @@ typedef struct http_filter_ctx
apr_off_t limit_used;
apr_int32_t chunk_used;
apr_int16_t chunkbits;
+ ap_mime_headers_ctx_t *mime_hdrs_ctx;
+ apr_bucket_brigade *tmp_bb;
enum
{
BODY_NONE, /* streamed data */
@@ -384,17 +386,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
apr_bucket_delete(e);
e = APR_BRIGADE_FIRST(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;
- }
-
+ again = 1; /* come around again */
break;
}
case BODY_NONE:
@@ -471,20 +465,23 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
}
case BODY_CHUNK_TRAILER: {
- rv = ap_get_brigade(f->next, b, mode, block, readbytes);
-
- /* for timeout */
- if (block == APR_NONBLOCK_READ
- && ((rv == APR_SUCCESS && APR_BRIGADE_EMPTY(b))
- || (APR_STATUS_IS_EAGAIN(rv)))) {
- return APR_EAGAIN;
+ if (ctx->tmp_bb == NULL) {
+ ctx->tmp_bb = apr_brigade_create(f->r->pool,
+ f->c->bucket_alloc);
}
-
+ else {
+ apr_brigade_cleanup(ctx->tmp_bb);
+ }
+ rv = ap_get_mime_headers_ex(f->r, ctx->tmp_bb,
f->r->footers_in,
+ f->next, block,
&ctx->mime_hdrs_ctx);
if (rv != APR_SUCCESS) {
return rv;
}
- break;
+ e = apr_bucket_eos_create(f->c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ ctx->eos_sent = 1;
+ return APR_SUCCESS;
}
default: {
break;
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1534668)
+++ modules/http/http_request.c (working copy)
@@ -463,6 +463,7 @@ static request_rec *internal_internal_redirect(con
new->main = r->main;
new->headers_in = r->headers_in;
+ new->footers_in = r->footers_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: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1534668)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -994,6 +994,7 @@ static request_rec *make_fake_req(conn_rec *c, req
rp->status = HTTP_OK;
rp->headers_in = apr_table_make(pool, 50);
+ rp->footers_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);
Index: include/http_protocol.h
===================================================================
--- include/http_protocol.h (revision 1534668)
+++ include/http_protocol.h (working copy)
@@ -75,6 +75,25 @@ AP_DECLARE(void) ap_get_mime_headers(request_rec *
AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r,
apr_bucket_brigade *bb);
+typedef struct ap_mime_headers_ctx_t ap_mime_headers_ctx_t;
+/**
+ * Full version of ap_get_mime_headers() that requires a
+ * temporary brigade, a blocking mode, a destination table
+ * and an non-blocking (optional) context to work with
+ * @param r The current request
+ * @param bb temp brigade
+ * @param to The table to put the headers/footers to
+ * @param from The filter to read from (r->proto_input_filters if NULL)
+ * @param block Read blocking mode
+ * @param pctx If not NULL, allows for reentrant non-blocking call
+ * @return APR_SUCCESS when done, APR_EAGAIN when non-blocking, or
+ * an unrecoverable APR error.
+ */
+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);
+
/* Finish up stuff after a request */
/**
@@ -632,6 +651,12 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
request_rec *r, int fold,
apr_bucket_brigade *bb);
+/** @see ap_rgetline */
+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 *from);
/**
* Get the method number associated with the given string, assumed to
* contain an HTTP method. Returns M_INVALID if not recognized.
Index: include/httpd.h
===================================================================
--- include/httpd.h (revision 1534668)
+++ include/httpd.h (working copy)
@@ -913,6 +913,8 @@ struct request_rec {
/** MIME header environment from the request */
apr_table_t *headers_in;
+ /** MIME footer environment from the request */
+ apr_table_t *footers_in;
/** MIME header environment for the response */
apr_table_t *headers_out;
/** MIME header environment for the response, printed even on errors
and
[EOS]
Re: stop copying footers to r->headers_in?
Posted by Eric Covener <co...@gmail.com>.
On Fri, Oct 25, 2013 at 9:12 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Oct 24, 2013 at 7:42 PM, Eric Covener <co...@gmail.com> wrote:
>>
>> On Wed, Oct 23, 2013 at 8:04 AM, Yann Ylavic <yl...@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
covener@gmail.com
Re: stop copying footers to r->headers_in?
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 24, 2013 at 7:42 PM, Eric Covener <co...@gmail.com> wrote:
> On Wed, Oct 23, 2013 at 8:04 AM, Yann Ylavic <yl...@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?
Posted by Eric Covener <co...@gmail.com>.
On Wed, Oct 23, 2013 at 8:04 AM, Yann Ylavic <yl...@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?
Posted by Yann Ylavic <yl...@gmail.com>.
Should I continue this way? Simply to propose patches?
On Sat, Oct 19, 2013 at 4:13 PM, Eric Covener <co...@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 = headers_in[counter].val;
+ if (old_cl_val) {
+ *old_cl_val = headers_in[counter].val;
+ }
continue;
}
/* for sub-requests, ignore freshness/expiry headers */
if (r->main) {
- if ( !strcasecmp(headers_in[counter].key, "If-Match")
+ if ( !strcasecmp(headers_in[counter].key, "If-Match")
|| !strcasecmp(headers_in[counter].key,
"If-Modified-Since")
|| !strcasecmp(headers_in[counter].key, "If-Range")
|| !strcasecmp(headers_in[counter].key,
"If-Unmodified-Since")
@@ -3351,9 +3374,11 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
headers_in[counter].val, CRLF,
NULL);
ap_xlate_proto_to_ascii(buf, strlen(buf));
- e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc);
+ e = apr_bucket_pool_create(buf, strlen(buf), p,
+ r->connection->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(header_brigade, e);
}
+
return OK;
}
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1534970)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -339,11 +339,11 @@ static int stream_reqbody_chunked(apr_pool_t *p,
bb = input_brigade;
}
- e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF
- /* <trailers> */
- ASCII_CRLF,
- 5, bucket_alloc);
+ e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF, 3, bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, e);
+ ap_proxy_fill_hdrbrgd(p, bb, r->footers_in, r, NULL, NULL);
+ e = apr_bucket_immortal_create(ASCII_CRLF, 2, bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
if (apr_table_get(r->subprocess_env, "proxy-sendextracrlf")) {
e = apr_bucket_immortal_create(ASCII_CRLF, 2, bucket_alloc);
@@ -994,6 +994,7 @@ static request_rec *make_fake_req(conn_rec *c, req
rp->status = HTTP_OK;
rp->headers_in = apr_table_make(pool, 50);
+ rp->footers_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);
Index: modules/proxy/mod_proxy.h
===================================================================
--- modules/proxy/mod_proxy.h (revision 1534970)
+++ modules/proxy/mod_proxy.h (working copy)
@@ -971,6 +971,21 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
char **old_te_val);
/**
+ * Fill a HTTP request header brigade, old_cl_val and old_te_val as
required.
+ * @param p pool
+ * @param header_brigade header brigade to fill
+ * @param header_table header table to use
+ * @param r request
+ * @param old_cl_val stored old content-len val
+ * @param old_te_val stored old TE val
+ * @return OK
+ */
+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);
+/**
* @param bucket_alloc bucket allocator
* @param r request
* @param p_conn proxy connection
[EOS]