On Sun, Mar 2, 2014 at 12:17 AM, Lieven Govaerts <lgo_at_apache.org> wrote:
> Update:
>
> On Fri, Feb 28, 2014 at 12:45 PM, Lieven Govaerts <lgo_at_apache.org> wrote:
>> Hi,
>>
>> On Wed, Feb 26, 2014 at 8:18 PM, Stefan Kueng <tortoisesvn_at_gmail.com> wrote:
>>> Hi,
>>>
>>> There's a crash happening in libsvn_ra_serf\util.c in the function
>>> svn_ra_serf__credentials_callback. There's a full crash dump available from
>>> here:
>>> https://www.crash-server.com/Problem.aspx?ClientID=tsvn&ProblemID=58311
>>>
>>> From what I can figure out from that crash dump is that the 'session' baton
>>> is not set (it's NULL), which leads to a segfault in the line:
>>>
>>> *username = apr_pstrdup(pool, session->proxy_username);
>>>
>>> The http return code is 407.
>>>
>>> But I can't figure out if this is a problem in serf or the svn library.
>>> Maybe one of you guys can have a look at this?
>>
>> I'm looking into this report but haven't reached a conclusion yet. The
>> dump doesn't contain a correct value for each function parameters and
>> local variables, so debugging isn't easy.
>>
>> The user is trying to connect over a http proxy to a https server; The
>> proxy requests username & password (Basic authn scheme). I see that
>> the baton passed to svn_ra_serf__credentials_callback isn't NULL, but
>> is a pointer to the ssl tunnel bucket context.
>>
>> This points me to serf__provide_credentials.
>>
>> Normally that function should setup the next request - the one
>> provided by the application that lead to the setup of the ssl tunnel -
>> so that the response_handler from that request can be passed to the
>> credentials callback.
>>
>> That mechanism must have failed. I see that the first request is
>> indeed a ssltunnel request, but the conn->state is SERF_CONN_INIT
>> instead of SERF_CONN_SETUP_SSLTUNNEL. This explains why the mechanism
>> to find and setup the first application request isn't triggered.
>>
>> The remaining question is how the connection can be in SERF_CONN_INIT
>> state while handling authentication of the response to the CONNECT
>> request. The only place where the connection state is reset is in
>> reset_connection, but I haven't found a scenario yet where that
>> function is called before an outstanding response on that connection
>> is handled.
>
> I see that the connection queue contains 3 requests:
> 1) a ssltunnel+priority request, handler_baton points to the ssltunnel context
> 2) a normal request, handler baton also points to the ssltunnel context
> 3) a normal request, handler_baton points to the svn_ra_serf
>
> That second request shouldn't be there. This is a requeued CONNECT
> request but without the ssltunnel flag set.
>
> I'm quite confident that the problem is the handling of a cancelled
> CONNECT request in ssltunnel.c/handle_response(). Besides the fact
> that it requeues a CONNECT request as normal request, ssltunnel
> requests should not be requeued in case of closed/reset connections,
> because a new CONNECT request will be created when opening the new
> socket.
>
> Unfortunately I can't imagine a scenario which triggers this code path
> (I've tried some), so I can't write a test case to simulate it.
I've finally found a matching scenario:
1. Serf sends a CONNECT request without authn headers, the (squid)
proxy responds with a 407 response.
2. Serf calls the application to get creds, sends a new CONNECT
request with authn headers
3. The server does not accept the credentials, responds with a 407 and
then closes the connection.
4. Serf reads APR_ECONNRESET from the socket (instead of the normal APR_EOF)
This assumes that squid does not close the connection on the initial
407, but only when incorrect credentials were provided. I don't know
if this is the case.
This also assumes that a closed connection can result in
APR_ECONNRESET. We know this to be true on Windows, serf already has
checks to handle this.
I can reproduce the crash on my Mac with a test triggering steps 1-3,
and if I alter the auth code to simulate step 4. Modifying the
ssltunnel code to not requeue the CONNECT request seems to fix the
issue. I'll cleanup and commit the patch later.
Lieven
>
>>
>>>
>>> Stefan
>>>
>>> --
>>> ___
>>> oo // \\ "De Chelonian Mobile"
>>> (_,\/ \_/ \ TortoiseSVN
>>> \ \_/_\_/> The coolest interface to (Sub)version control
>>> /_/ \_\ http://tortoisesvn.n
Received on 2014-03-02 11:45:58 CET