Re: [PATCH] http: fix segfault in handle_curl_result

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> When we create an http active_request_slot, we can set its
> "results" pointer back to local storage. The http code will
> fill in the details of how the request went, and we can
> access those details even after the slot has been cleaned
> up.
> ...
> However, I'm leaving that out of this patch.  Commit 8809703 was
> supposed to be a refactor with zero behavior changes, but it regressed.
> This fixes the regression by behaving exactly as we did beforehand. We
> can build the other thing on top.

Thanks for a good write-up.

I agree with the fix, and I also agree that it does not make sense
to pass slot to handle-curl-RESULT when we know the slot may not
have anything to do with the result we are dealing with.  If
anything, we should be passing result (which this patch already
does) and curl handle (the sole reason why slot is passed, after
this patch, becomes to let the function access it via slot->curl),
but as your analysis shows, with dfa1725 (post 1.7.10.2) it should
not be even needed to call init_curl_http_auth() here, which is the
[2/2] of your follow-up.

Happy I am.  Thanks.

>  http.c        | 7 +++----
>  http.h        | 3 ++-
>  remote-curl.c | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/http.c b/http.c
> index 7c4a407..9334386 100644
> --- a/http.c
> +++ b/http.c
> @@ -744,10 +744,9 @@ int handle_curl_result(struct active_request_slot *slot)
>  	return strbuf_detach(&buf, NULL);
>  }
>  
> -int handle_curl_result(struct active_request_slot *slot)
> +int handle_curl_result(struct active_request_slot *slot,
> +		       struct slot_results *results)
>  {
> -	struct slot_results *results = slot->results;
> -
>  	if (results->curl_result == CURLE_OK) {
>  		credential_approve(&http_auth);
>  		return HTTP_OK;
> @@ -818,7 +817,7 @@ static int http_request(const char *url, void *result, int target, int options)
>  
>  	if (start_active_slot(slot)) {
>  		run_active_slot(slot);
> -		ret = handle_curl_result(slot);
> +		ret = handle_curl_result(slot, &results);
>  	} else {
>  		error("Unable to start HTTP request for %s", url);
>  		ret = HTTP_START_FAILED;
> diff --git a/http.h b/http.h
> index 12de255..0bd1e84 100644
> --- a/http.h
> +++ b/http.h
> @@ -78,7 +78,8 @@ extern void finish_all_active_slots(void);
>  extern void run_active_slot(struct active_request_slot *slot);
>  extern void finish_active_slot(struct active_request_slot *slot);
>  extern void finish_all_active_slots(void);
> -extern int handle_curl_result(struct active_request_slot *slot);
> +extern int handle_curl_result(struct active_request_slot *slot,
> +			      struct slot_results *results);
>  
>  #ifdef USE_CURL_MULTI
>  extern void fill_active_slots(void);
> diff --git a/remote-curl.c b/remote-curl.c
> index 3ec474f..6054e47 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -369,7 +369,7 @@ static int run_slot(struct active_request_slot *slot)
>  	slot->curl_result = curl_easy_perform(slot->curl);
>  	finish_active_slot(slot);
>  
> -	err = handle_curl_result(slot);
> +	err = handle_curl_result(slot, &results);
>  	if (err != HTTP_OK && err != HTTP_REAUTH) {
>  		error("RPC failed; result=%d, HTTP code = %ld",
>  		      results.curl_result, results.http_code);