[
https://issues.apache.org/jira/browse/SERF-195?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18012364#comment-18012364
]
Branko Čibej commented on SERF-195:
-----------------------------------
Patch committed to {{branches/SERF-195}} in r1927641.
> [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
> Assignee: Branko Čibej
> Priority: Major
> 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.20.10#820010)
