On Sat, Jun 22, 2013 at 7:32 PM, Lieven Govaerts <svn...@mobsol.be> wrote:
> 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
Unfortunately the attached patch was not entirely correct, even though
for svn it seems to work ok, it breaks the new ssltunnel unit test.
Attached an updated patch. I'll probably do some more testing this
weekend, and commit any improvements to serf trunk.
Lieven
> On Fri, Jun 21, 2013 at 8:55 AM, Lieven Govaerts <svn...@mobsol.be> wrote:
>> Follow up also to serf-dev.
>>
>> On Thu, Jun 20, 2013 at 11:05 PM, Lieven Govaerts <svn...@mobsol.be> wrote:
>>> On Thu, Jun 20, 2013 at 10:30 PM, Greg Stein <gst...@gmail.com> wrote:
>>>> On Thu, Jun 20, 2013 at 2:19 PM, Stefan Küng <tortoise...@gmail.com> 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,20 @@ 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 = serf_connection_priority_request_create(conn,
+ setup,
+ setup_baton);
+ request->ssltunnel = 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
