Hi,
On Sat, Jan 10, 2015 at 1:07 PM, Philip Martin
<philip.mar...@wandisco.com> wrote:
> Philip Martin <philip.mar...@wandisco.com> writes:
>
>> Philip Martin <philip.mar...@wandisco.com> writes:
>>
>>> While debugging a problem reported on users I accidentally sent an extra
>>> byte to the client: I sent Content-Length of N and then sent N+1 bytes.
>>> The first N bytes made a valid response, so serf was happy at that
>>> stage. When processing the next request the extra byte causes serf to
>>> attempt to handle the spurious data before any request handler is setup.
>>> This is with serf 1.3.x@2440.
>>
>> Here's a patch that causes serf to return an error if this sort of
>> spurious data is received:
>>
>> Index: outgoing.c
>> ===================================================================
>> --- outgoing.c (revision 2445)
>> +++ outgoing.c (working copy)
>> @@ -1109,7 +1109,11 @@ static apr_status_t read_from_connection(serf_conn
>> goto error;
>> }
>>
>> - /* Unexpected response from the server */
>> + /* Unexpected response from the server. This can happen if
>> + * a buggy server sends more than Content-Length data.
>> + */
>> + status = SERF_ERROR_BAD_HTTP_RESPONSE;
>> + goto error;
>>
>> }
>
> That breaks HTTPS negotiation. This works:
Does serf-'s test suite capture this situation?
What do you see here? I suppose status == APR_SUCCESS with 0 bytes of data?
>
> Index: outgoing.c
> ===================================================================
> --- outgoing.c (revision 2445)
> +++ outgoing.c (working copy)
> @@ -1109,8 +1109,14 @@
> goto error;
> }
>
> - /* Unexpected response from the server */
> -
> + if (!request->req_bkt) {
> + /* Unexpected response from the server. This can happen if
> + * a buggy server sends more than Content-Length data.
> + */
> + status = SERF_ERROR_BAD_HTTP_RESPONSE;
> + goto error;
> + }
> +
> }
>
> /* If the request doesn't have a response bucket, then call the
You're trying to capture the situation where serf has:
- finished writing a request ( req_bkt == NULL && writing_started )
- has no more requests in the pending queue
- and receives actual bytes of data.
Right?
Looks like your patch is a correct way to fix the problem. Some
comments would be welcome, and if you have the time a test.
I should have some time this weekend to have a go at a test and commit.
Lieven
