Alex W created SERF-195:
---------------------------
Summary: [PATCH] time-consuming operations in auth_ module leads
to memory corruption
Key: SERF-195
URL: https://issues.apache.org/jira/browse/SERF-195
Project: serf
Issue Type: Bug
Affects Versions: serf-1.4.0, serf-trunk
Reporter: Alex W
Attachments: linked-list-fix.patch
In serf trunk and 1.4.x, doing time-consuming operations in an auth module's
"setup" function can lead to memory corruption and crashes. I strongly suspect
this can be triggered in other ways as well, if they can lead to delayed
requests, timeouts or errors on the server, etc. You can easily reproduce this
(pretty reliably) by making a change such as:
{noformat}
diff a/auth/auth_basic.c b/auth/auth_basic.c
--- a/auth/auth_basic.c
+++ b/auth/auth_basic.c
@@ -169,6 +169,7 @@ serf__setup_request_basic_auth(const serf__authn_scheme_t
*scheme,
basic_info = authn_info->baton;
if (basic_info && basic_info->header && basic_info->value) {
+ usleep(75000);
serf_bucket_headers_setn(hdrs_bkt, basic_info->header,
basic_info->value);
return APR_SUCCESS;
{noformat}
And then using serf via e.g. {{svn co}} of a large repository.
The crash which occurs results from the {{written_reqs}} and {{unwritten_reqs}}
lists on the {{serf_connection_t}} containing invalid pointers. Generally,
since the memory is pooled, we don't notice the corruption until teardown time.
If you run with pool debugging and valgrind you can eventually narrow down the
cause to the linked lists.
Several places which manipulate the linked lists are suspicious, e.g. this
section in {{outgoing.c}}:
{noformat}
serf__link_requests(&conn->written_reqs, &conn->written_reqs_tail,
request);
conn->nr_of_written_reqs++;
conn->unwritten_reqs = conn->unwritten_reqs->next;
conn->nr_of_unwritten_reqs--;
request->next = NULL;
{noformat}
This assumes (but never checks) that the request in question is at the front of
the {{unwritten_reqs}} list, which can be violated in both error cases and in
cases of aborted requests. The ordering of operations here (link the request
onto the new list, then unlink it from the old one) should also make a reader
pretty suspicious, though in this particular case it shouldn't be a problem
(since {{serf__link_requests}} doesn't actually change the {{next}} pointer).
There are also other sections of linked list code too which are a little
questionable.
I'm attaching a patch which reorganises the {{written_reqs}} and
{{unwritten_reqs}} lists into their own structs and provides higher-level
functions for manipulating them. These also add assertions to make sure that
the correct list is being used. I also check conditions in places such as this
section of {{outgoing.c}} for other error cases. After these changes I can no
longer reproduce the incorrect free() or other crashes with a slow auth plugin.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
