You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2019/10/18 08:29:27 UTC
Re: svn commit: r1868576 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c server/protocol.c
On 10/18/2019 09:50 AM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Oct 18 07:50:59 2019
> New Revision: 1868576
>
> URL: http://svn.apache.org/viewvc?rev=1868576&view=rev
> Log:
> mod_proxy_http: Fix 100-continue deadlock for spooled request bodies. PR 63855.
>
> Send "100 Continue", if needed, before fetching/blocking on the request body in
> spool_reqbody_cl(), otherwise mod_proxy and the client can wait for each other,
> leading to a request timeout (408).
>
> While at it, make so that ap_send_interim_response() uses the default status
> line if none is set in r->status_line.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1868576&r1=1868575&r2=1868576&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Fri Oct 18 07:50:59 2019
> @@ -431,6 +431,21 @@ static int spool_reqbody_cl(proxy_http_r
> apr_file_t *tmpfile = NULL;
> apr_off_t limit;
>
> + /* Send "100 Continue" now if the client expects one, before
> + * blocking on the body, otherwise we'd wait for each other.
> + */
> + if (req->expecting_100) {
> + int saved_status = r->status;
> +
> + r->expecting_100 = 1;
> + r->status = HTTP_CONTINUE;
> + ap_send_interim_response(r, 0);
> + AP_DEBUG_ASSERT(!r->expecting_100);
> +
> + r->status = saved_status;
> + req->expecting_100 = 0;
> + }
Why sending it directly and not just set r->expecting_100 = 1; and let the HTTP_INPUT filter do the job?
So
/*
* Tell the HTTP_INPUT filter that it should send a "100 continue" if the client
* expects one, before blocking on the body, otherwise we'd wait for each other.
*/
if (req->expecting_100) {
r->expecting_100 = 1;
req->expecting_100 = 0;
}
Regards
Rüdiger
Re: svn commit: r1868576 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Nov 4, 2019 at 10:06 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> Sorry for getting back late. I missed that the HTTP_IN filter only sends a possible
> 100 continue during the first run. Good catch. Nevertheless this leaves the question open
> how to solve this. Either by your patch which sends the intermediate response itself
> or by fixing HTTP_IN to check for the need to sent it not only on the first run.
> I am undecided. Maybe someone else has a strong opinion either way otherwise just move on with your patch.
Not someone else but still \o_
Actually I like the idea of HTTP_IN to handle 100 continue for others
cases than the first time around, it greatly simplifies things
(including for mod_proxy_http).
The way I see it is to move 100 continue handling out of the first
time only block, as you propose, and to change the decision for when
it's done like:
/* Since we're about to read data, send 100-Continue if needed.
- * Only valid on chunked and C-L bodies where the C-L is > 0. */
- if ((ctx->state == BODY_CHUNK
- || (ctx->state == BODY_LENGTH && ctx->remaining > 0))
+ * Only valid on chunked and C-L bodies where the C-L is > 0.
+ *
+ * If the read is to be nonblocking though, the caller may not want to
+ * handle this just now (e.g. mod_proxy_http), and is prepared to read
+ * nothing should the client really waits for 100 continue, so we can
+ * avoid sending it now and wait for the next blocking read.
+ *
+ * Also, we track in ctx->data_read whether something is read below,
+ * which allows to not send 100 continue either in this case, according
+ * to https://tools.ietf.org/html/rfc7231#section-5.1.1 :
+ * A server MAY omit sending a 100 (Continue) response if it has
+ * already received some or all of the message body for the
+ * corresponding request, ...
+ *
+ * In any case, even if r->expecting remains set at the end of the
+ * request processing, ap_set_keepalive() will finally do the right
+ * thing (i.e. "Connection: close" the connection).
+ */
+ if (block == APR_BLOCK_READ
+ && (ctx->state == BODY_CHUNK
+ || (ctx->state == BODY_LENGTH && ctx->remaining > 0))
&& f->r->expecting_100 && f->r->proto_num >= HTTP_VERSION(1,1)
- && !(f->r->eos_sent || f->r->bytes_sent)) {
+ && !(ctx->data_read || f->r->eos_sent || f->r->bytes_sent)) {
Full patch attached, changes for both HTTP_IN and mod_proxy_http (this
is a POC only, untested yet).
I like the way it removes all the r->100_continue => req->100_continue
=> r->100_continue danse in mod_proxy_http (since HTTP_IN now does
what it wants in nonblocking mode), while the changes in HTTP_IN do
not look to insane (note the use of ap_send_interim_response() instead
of open coding it)...
WDYT?
Regards,
Yann.
Re: svn commit: r1868576 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/20/2019 04:52 PM, Yann Ylavic wrote:
> Hi Rüdiger,
>
> On Sat, Oct 19, 2019 at 4:04 PM Yann Ylavic <yl...@gmail.com> wrote:
>>
>> /*
>> * Tell the HTTP_IN filter that it should send a "100 continue" if the
>> * client expects one, before blocking on the body, otherwise we'd wait
>> * for each other.
>> */
>> if (req->expecting_100) {
>> /* From https://tools.ietf.org/html/rfc7231#section-5.1.1
>> * A server MAY omit sending a 100 (Continue) response if it has
>> * already received some or all of the message body for the
>> * corresponding request, or if [snip].
>> */
>> r->expecting_100 = APR_BRIGADE_EMPTY(input_brigade);
>> req->expecting_100 = 0;
>> }
>
> I had to revert this, the HTTP_IN filter won't handle "100 continue"
> anymore after prefetching..
> So back to this r1868576, plus r1868653 to handle
> rfc7231#section-5.1.1's MAY directly in ap_proxy_http_prefetch(), when
> we know we have some body bytes.
>
> Looks good to you?
Sorry for getting back late. I missed that the HTTP_IN filter only sends a possible
100 continue during the first run. Good catch. Nevertheless this leaves the question open
how to solve this. Either by your patch which sends the intermediate response itself
or by fixing HTTP_IN to check for the need to sent it not only on the first run.
I am undecided. Maybe someone else has a strong opinion either way otherwise just move on with your patch.
Regards
Rüdiger
Re: svn commit: r1868576 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
Hi Rüdiger,
On Sat, Oct 19, 2019 at 4:04 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> /*
> * Tell the HTTP_IN filter that it should send a "100 continue" if the
> * client expects one, before blocking on the body, otherwise we'd wait
> * for each other.
> */
> if (req->expecting_100) {
> /* From https://tools.ietf.org/html/rfc7231#section-5.1.1
> * A server MAY omit sending a 100 (Continue) response if it has
> * already received some or all of the message body for the
> * corresponding request, or if [snip].
> */
> r->expecting_100 = APR_BRIGADE_EMPTY(input_brigade);
> req->expecting_100 = 0;
> }
I had to revert this, the HTTP_IN filter won't handle "100 continue"
anymore after prefetching..
So back to this r1868576, plus r1868653 to handle
rfc7231#section-5.1.1's MAY directly in ap_proxy_http_prefetch(), when
we know we have some body bytes.
Looks good to you?
Regards,
Yann.
Re: svn commit: r1868576 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Oct 18, 2019 at 2:37 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 10/18/2019 01:50 PM, Yann Ylavic wrote:
> >
> > I thought about it but if the client somehow sent both the header
> > (with "Expect: 100-continue") and the body altogether, we have the
> > body prefetched already and spool_reqbody_cl() will not call
> > stream_reqbody_read(), thus HTTP_IN filter.
>
> Correct. But in this case we could sent the final status code directly or we sent a 100 continue before.
> IMHO it does not matter. At least this my read of https://tools.ietf.org/html/rfc7231#section-5.1.1
Fair enough. I finally went for:
/*
* Tell the HTTP_IN filter that it should send a "100 continue" if the
* client expects one, before blocking on the body, otherwise we'd wait
* for each other.
*/
if (req->expecting_100) {
/* From https://tools.ietf.org/html/rfc7231#section-5.1.1
* A server MAY omit sending a 100 (Continue) response if it has
* already received some or all of the message body for the
* corresponding request, or if [snip].
*/
r->expecting_100 = APR_BRIGADE_EMPTY(input_brigade);
req->expecting_100 = 0;
}
such that we are fine with further processing (and the RFC).
Thanks for the review Rüdiger!
Regards,
Yann.
Re: svn commit: r1868576 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/18/2019 01:50 PM, Yann Ylavic wrote:
> On Fri, Oct 18, 2019 at 10:29 AM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 10/18/2019 09:50 AM, ylavic@apache.org wrote:
>>>
>>> + /* Send "100 Continue" now if the client expects one, before
>>> + * blocking on the body, otherwise we'd wait for each other.
>>> + */
>>> + if (req->expecting_100) {
>>> + int saved_status = r->status;
>>> +
>>> + r->expecting_100 = 1;
>>> + r->status = HTTP_CONTINUE;
>>> + ap_send_interim_response(r, 0);
>>> + AP_DEBUG_ASSERT(!r->expecting_100);
>>> +
>>> + r->status = saved_status;
>>> + req->expecting_100 = 0;
>>> + }
>>
>> Why sending it directly and not just set r->expecting_100 = 1; and let the HTTP_INPUT filter do the job?
>
> I thought about it but if the client somehow sent both the header
> (with "Expect: 100-continue") and the body altogether, we have the
> body prefetched already and spool_reqbody_cl() will not call
> stream_reqbody_read(), thus HTTP_IN filter.
Correct. But in this case we could sent the final status code directly or we sent a 100 continue before.
IMHO it does not matter. At least this my read of https://tools.ietf.org/html/rfc7231#section-5.1.1
- Requirements for servers.
And even if you are worried I guess we could do
/*
* Tell the HTTP_INPUT filter that it should send a "100 continue" if the client
* expects one, before blocking on the body, otherwise we'd wait for each other.
*/
if (req->expecting_100 && APR_BRIGADE_EMPTY(input_brigade)) {
r->expecting_100 = 1;
req->expecting_100 = 0;
}
Regards
Rüdiger
Re: svn commit: r1868576 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_http.c server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Oct 18, 2019 at 10:29 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 10/18/2019 09:50 AM, ylavic@apache.org wrote:
> >
> > + /* Send "100 Continue" now if the client expects one, before
> > + * blocking on the body, otherwise we'd wait for each other.
> > + */
> > + if (req->expecting_100) {
> > + int saved_status = r->status;
> > +
> > + r->expecting_100 = 1;
> > + r->status = HTTP_CONTINUE;
> > + ap_send_interim_response(r, 0);
> > + AP_DEBUG_ASSERT(!r->expecting_100);
> > +
> > + r->status = saved_status;
> > + req->expecting_100 = 0;
> > + }
>
> Why sending it directly and not just set r->expecting_100 = 1; and let the HTTP_INPUT filter do the job?
I thought about it but if the client somehow sent both the header
(with "Expect: 100-continue") and the body altogether, we have the
body prefetched already and spool_reqbody_cl() will not call
stream_reqbody_read(), thus HTTP_IN filter.
I think at some point the "100 continue" will be sent (e.g. when
"forwarding" the one from the backend), but I felt that leaving
r->expecting = 1 until then was asking for troubles (possibly)...
>
> So
>
> /*
> * Tell the HTTP_INPUT filter that it should send a "100 continue" if the client
> * expects one, before blocking on the body, otherwise we'd wait for each other.
> */
> if (req->expecting_100) {
> r->expecting_100 = 1;
> req->expecting_100 = 0;
> }
That would be nicer, though :/
Regards,
Yann.