Stefan,
attached patch to serf 1.2.1 should solve this particular type of
crash you reported.
The patch is made against a serf 1.2.x working copy as follows:
$ svn merge ^/trunk -c 1943,1944
On Fri, Jun 21, 2013 at 8:55 AM, Lieven Govaerts <[email protected]> wrote:
> Follow up also to serf-dev.
>
> On Thu, Jun 20, 2013 at 11:05 PM, Lieven Govaerts <[email protected]> wrote:
>> On Thu, Jun 20, 2013 at 10:30 PM, Greg Stein <[email protected]> wrote:
>>> On Thu, Jun 20, 2013 at 2:19 PM, Stefan Küng <[email protected]> wrote:
>>>> Hi,
>>>>
>>>> Another crash that's climbing up in the crash report statistics for TSVN.
>>>> Seems to be related to the previously discussed problem with checkouts in
>>>> TSVN.
>>
>> Thanks Stefan.
>>
>>>> The stack trace:
>>>>
>>>> BowPad
>>>>
>>>> libsvn_tsvn.dll!svn_ra_serf__credentials_callback(char * *
>>>> username=0xffffffffffffffff, char * * password=0x0000000002ba0210,
>>>> serf_request_t * request=0x0000000002ba0280, void *
>>>> baton=0x0000000002c12588, int code=407, const char *
>>>> authn_type=0x000007fee0bf0b58, const char * realm=0x0000000002c12b60,
>>>> apr_pool_t * pool=0x0000000002bb8258) Line 1789 C
>>>
[..]
>>>
>>
>> Looks like it's the authentication handling when setting up a SSL
>> tunnel that's at fault here, at least I can easily reproduce it with
>> an apache http proxy connetion to a https repo.
>>
>> The ssl tunnel is started by a CONNECT request created by serf. When
>> the proxy requests credentials, serf will call back to the
>> application. As the application doesn't know about this request, it
>> doesn't get a valid baton either, so can't get baton->session ...
>>
>> That baton it gets is the ctx used by the ssltunnel code.
>>
>> Hm, have to think about how we can solve this. Not sure it can be done
>> with the existing API.
>
The patch implements an alternative fix for the issue that does not
require a new API, so if it works for you we can include it in 1.2.2.
[..]
Lieven
Index: auth/auth_basic.c
===================================================================
--- auth/auth_basic.c (revision 1938)
+++ auth/auth_basic.c (working copy)
@@ -84,9 +84,11 @@ serf__handle_basic_auth(int code,
/* Ask the application for credentials */
apr_pool_create(&cred_pool, pool);
- status = (*ctx->cred_cb)(&username, &password, request, baton,
- code, authn_info->scheme->name,
- authn_info->realm, cred_pool);
+ status = serf__provide_credentials(ctx,
+ &username, &password,
+ request, baton,
+ code, authn_info->scheme->name,
+ authn_info->realm, cred_pool);
if (status) {
apr_pool_destroy(cred_pool);
return status;
Index: auth/auth_digest.c
===================================================================
--- auth/auth_digest.c (revision 1938)
+++ auth/auth_digest.c (working copy)
@@ -294,9 +294,11 @@ serf__handle_digest_auth(int code,
/* Ask the application for credentials */
apr_pool_create(&cred_pool, pool);
- status = (*ctx->cred_cb)(&username, &password, request, baton,
- code, authn_info->scheme->name,
- authn_info->realm, cred_pool);
+ status = serf__provide_credentials(ctx,
+ &username, &password,
+ request, baton,
+ code, authn_info->scheme->name,
+ authn_info->realm, cred_pool);
if (status) {
apr_pool_destroy(cred_pool);
return status;
Index: outgoing.c
===================================================================
--- outgoing.c (revision 1938)
+++ outgoing.c (working copy)
@@ -628,6 +628,29 @@ static apr_status_t prepare_conn_streams(serf_conn
return APR_SUCCESS;
}
+static apr_status_t setup_request(serf_request_t *request)
+{
+ serf_connection_t *conn = request->conn;
+ apr_status_t status;
+
+ /* Now that we are about to serve the request, allocate a pool. */
+ apr_pool_create(&request->respool, conn->pool);
+ request->allocator = serf_bucket_allocator_create(request->respool,
+ NULL, NULL);
+ apr_pool_cleanup_register(request->respool, request,
+ clean_resp, clean_resp);
+
+ /* Fill in the rest of the values for the request. */
+ status = request->setup(request, request->setup_baton,
+ &request->req_bkt,
+ &request->acceptor,
+ &request->acceptor_baton,
+ &request->handler,
+ &request->handler_baton,
+ request->respool);
+ return status;
+}
+
/* write data out to the connection */
static apr_status_t write_to_connection(serf_connection_t *conn)
{
@@ -717,27 +740,14 @@ static apr_status_t write_to_connection(serf_conne
}
if (request->req_bkt == NULL) {
- /* Now that we are about to serve the request, allocate a pool. */
- apr_pool_create(&request->respool, conn->pool);
- request->allocator = serf_bucket_allocator_create(request->respool,
- NULL, NULL);
- apr_pool_cleanup_register(request->respool, request,
- clean_resp, clean_resp);
-
- /* Fill in the rest of the values for the request. */
- read_status = request->setup(request, request->setup_baton,
- &request->req_bkt,
- &request->acceptor,
- &request->acceptor_baton,
- &request->handler,
- &request->handler_baton,
- request->respool);
-
+ read_status = setup_request(request);
if (read_status) {
/* Something bad happened. Propagate any errors. */
return read_status;
}
+ }
+ if (!request->written) {
request->written = 1;
serf_bucket_aggregate_append(ostreamt, request->req_bkt);
}
@@ -895,6 +905,57 @@ static apr_status_t handle_async_response(serf_con
return status;
}
+
+apr_status_t
+serf__provide_credentials(serf_context_t *ctx,
+ char **username,
+ char **password,
+ serf_request_t *request, void *baton,
+ int code, const char *authn_type,
+ const char *realm,
+ apr_pool_t *pool)
+{
+ serf_connection_t *conn = request->conn;
+ serf_request_t *authn_req = request;
+ apr_status_t status;
+
+ if (request->ssltunnel == 1 &&
+ conn->state == SERF_CONN_SETUP_SSLTUNNEL) {
+ /* This is a CONNECT request to set up an SSL tunnel over a proxy.
+ This request is created by serf, so if the proxy requires
+ authentication, we can't ask the application for credentials with
+ this request.
+
+ Solution: setup the first request created by the application on
+ this connection, and use that request and its handler_baton to
+ call back to the application. */
+
+ authn_req = request->next;
+ /* assert: app_request != NULL */
+ if (!authn_req)
+ return APR_EGENERAL;
+
+ if (!authn_req->req_bkt) {
+ apr_status_t status;
+
+ status = setup_request(authn_req);
+ /* If we can't setup a request, don't bother setting up the
+ ssl tunnel. */
+ if (status)
+ return status;
+ }
+ }
+
+ /* Ask the application. */
+ status = (*ctx->cred_cb)(username, password,
+ authn_req, authn_req->handler_baton,
+ code, authn_type, realm, pool);
+ if (status)
+ return status;
+
+ return APR_SUCCESS;
+}
+
/* read data from the connection */
static apr_status_t read_from_connection(serf_connection_t *conn)
{
@@ -1330,11 +1391,12 @@ void serf_connection_set_async_responses(
conn->async_handler_baton = handler_baton;
}
-
-serf_request_t *serf_connection_request_create(
- serf_connection_t *conn,
- serf_request_setup_t setup,
- void *setup_baton)
+static serf_request_t *
+create_request(serf_connection_t *conn,
+ serf_request_setup_t setup,
+ void *setup_baton,
+ int priority,
+ int ssltunnel)
{
serf_request_t *request;
@@ -1346,10 +1408,25 @@ void serf_connection_set_async_responses(
request->respool = NULL;
request->req_bkt = NULL;
request->resp_bkt = NULL;
- request->priority = 0;
+ request->priority = priority;
request->written = 0;
+ request->ssltunnel = ssltunnel;
request->next = NULL;
+ return request;
+}
+
+serf_request_t *serf_connection_request_create(
+ serf_connection_t *conn,
+ serf_request_setup_t setup,
+ void *setup_baton)
+{
+ serf_request_t *request;
+
+ request = create_request(conn, setup, setup_baton,
+ 0, /* priority */
+ 0 /* ssl tunnel */);
+
/* Link the request to the end of the request chain. */
link_requests(&conn->requests, &conn->requests_tail, request);
@@ -1369,17 +1446,9 @@ serf_request_t *serf_connection_priority_request_c
serf_request_t *request;
serf_request_t *iter, *prev;
- request = serf_bucket_mem_alloc(conn->allocator, sizeof(*request));
- request->conn = conn;
- request->setup = setup;
- request->setup_baton = setup_baton;
- request->handler = NULL;
- request->respool = NULL;
- request->req_bkt = NULL;
- request->resp_bkt = NULL;
- request->priority = 1;
- request->written = 0;
- request->next = NULL;
+ request = create_request(conn, setup, setup_baton,
+ 1, /* priority */
+ 0 /* ssl tunnel */);
/* Link the new request after the last written request. */
iter = conn->requests;
@@ -1413,6 +1482,26 @@ serf_request_t *serf_connection_priority_request_c
}
+serf_request_t *serf__ssltunnel_request_create(serf_connection_t *conn,
+ serf_request_setup_t setup,
+ void *setup_baton)
+{
+ serf_request_t *request;
+
+ request = create_request(conn, setup, setup_baton,
+ 1, /* priority */
+ 1 /* ssl tunnel */);
+
+ /* Link the request to the end of the request chain. */
+ link_requests(&conn->requests, &conn->requests_tail, request);
+
+ /* Ensure our pollset becomes writable in context run */
+ conn->ctx->dirty_pollset = 1;
+ conn->dirty_conn = 1;
+
+ return request;
+}
+
apr_status_t serf_request_cancel(serf_request_t *request)
{
return cancel_request(request, &request->conn->requests, 0);
Index: serf_private.h
===================================================================
--- serf_private.h (revision 1938)
+++ serf_private.h (working copy)
@@ -76,6 +76,8 @@ struct serf_request_t {
int written;
int priority;
+ /* 1 if this is a request to setup a SSL tunnel, 0 for normal requests. */
+ int ssltunnel;
/* This baton is currently only used for digest authentication, which
needs access to the uri of the request in the response handler.
@@ -380,6 +382,17 @@ apr_status_t serf__open_connections(serf_context_t
apr_status_t serf__process_connection(serf_connection_t *conn,
apr_int16_t events);
apr_status_t serf__conn_update_pollset(serf_connection_t *conn);
+serf_request_t *serf__ssltunnel_request_create(serf_connection_t *conn,
+ serf_request_setup_t setup,
+ void *setup_baton);
+apr_status_t serf__provide_credentials(serf_context_t *ctx,
+ char **username,
+ char **password,
+ serf_request_t *request,
+ void *baton,
+ int code, const char *authn_type,
+ const char *realm,
+ apr_pool_t *pool);
/* from ssltunnel.c */
apr_status_t serf__ssltunnel_connect(serf_connection_t *conn);
Index: ssltunnel.c
===================================================================
--- ssltunnel.c (revision 1938)
+++ ssltunnel.c (working copy)
@@ -167,9 +167,9 @@ apr_status_t serf__ssltunnel_connect(serf_connecti
conn);
/* TODO: should be the first request on the connection. */
- serf_connection_priority_request_create(conn,
- setup_request,
- ctx);
+ serf__ssltunnel_request_create(conn,
+ setup_request,
+ ctx);
conn->state = SERF_CONN_SETUP_SSLTUNNEL;
serf__log(CONN_VERBOSE, __FILE__,
Index: .
===================================================================
--- . (revision 1938)
+++ . (working copy)
Property changes on: .
___________________________________________________________________
Modified: svn:mergeinfo
Merged /trunk:r1943-1944
