[pacman-dev] segfault after curl_easy_cleanup via progress callback

[Dan McGee]

On Wed, Aug 10, 2011 at 5:48 PM, Dave Reisner <d at falconindy.com> wrote:
> Hi all,
>
> I received a bug report with backtrace [1] for a curl powered app that I
> work on. It shows a segfault in a progress callback, which is triggered
> _after_ curl_easy_cleanup has been called. Unfortunately, I can't
> reproduce this, and it's still a development version of pacman with a
> limited userbase. There's a half dozen devs using this on a daily basis,
> and this is a routine operation that one of us would have surely run
> into by now.
>
> That said, the bug reporter was able to find another bug report[2] with
> a very similar backtrace. Note that this bug report uses an earlier
> version of curl, I believe 7.20, while we're using 7.21.7.
>
> The documentation for curl_easy_cleanup states that further calls to the
> handle are invalid after this call so I'm a bit confused as to why curl
> would be hitting the progress callback at this point. This indirectly
> _insists_ that the application touches the handle. Am I misunderstanding
> the expectation of curl_easy_cleanup that it's _possible_ due to an
> unfinished transfer that the progress callback may be hit one final
> time?
>
> In case it's relevant, I'll point to the where the majority of our curl
> implementation lies [3], but I'm somewhat confident that pacman "does
> the right thing" here.
>
> If anyone can point me in the right direction, I've be hugely
> appreciative.
>
> Thanks,
> Dave Reisner
>
> [1] http://pastebin.com/Pef1Qedq
> [2] http://www.crosswire.org/bugs/browse/API-128
> [3] https://github.com/falconindy/pacman/blob/master/lib/libalpm/dload.c#L180

Took a stab at this. Taking the stacktrace from [1], the only missing
values that make sense and I can reconstruct are like the following:
#1  0xb7dd8776 in Curl_pgrsUpdate () from /usr/lib/libcurl.so.4
#2  0xb7e12275 in Curl_pp_easy_statemach () from /usr/lib/libcurl.so.4
#3  0xb7de2088 in ?? () from /usr/lib/libcurl.so.4 (***ftp_easy_statemach)
#4  0xb7de21a9 in ?? () from /usr/lib/libcurl.so.4 (***ftp_quit
inlined in ftp_disconnect)
#5  0xb7de8547 in Curl_disconnect () from /usr/lib/libcurl.so.4#1
0xb7dd8776 in Curl_pgrsUpdate () from /usr/lib/libcurl.so.4

I don't see anywhere we explicitly disconnect progress meters from
connections, so my stab at that is below. I still think there is some
sort of underlying state machine problem though as we shouldn't ever
be calling progress from the state where we are calling QUIT on an FTP
connection.

-Dan McGee

$ git diff | cat
diff --git a/lib/url.c b/lib/url.c
index f888c1d..c9d4bc9 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -435,6 +435,9 @@ CURLcode Curl_close(struct SessionHandle *data)
 #endif

   Curl_expire(data, 0); /* shut off timers */
+  /* disconnect any progress callback */
+  data->set.fprogress = NULL;
+  data->progress.callback = FALSE;

   if(m)
     /* This handle is still part of a multi handle, take care of this first