Mailing List Archive

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 [ In reply to ]
On Fri, Oct 18, 2019 at 10:29 AM Ruediger Pluem <rpluem@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.
Re: svn commit: r1868576 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c server/protocol.c [ In reply to ]
On 10/18/2019 01:50 PM, Yann Ylavic wrote:
> On Fri, Oct 18, 2019 at 10:29 AM Ruediger Pluem <rpluem@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 [ In reply to ]
On Fri, Oct 18, 2019 at 2:37 PM Ruediger Pluem <rpluem@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 [ In reply to ]
Hi Rüdiger,

On Sat, Oct 19, 2019 at 4:04 PM Yann Ylavic <ylavic.dev@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.