Re: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 26, 2022, 21:59 UTC
Message-ID: <YfHERK0RUwVjM963@nand.local>
In-Reply-To: <patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com>

On Wed, Jan 26, 2022 at 10:30:40PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 16 quoted lines

> But we can instead amend the code added in baa7b67d091 (HTTP slot
> reuse fixes, 2006-03-10) to get rid of "int *finished" entirely. I
> instrumented the code to add this after every use of slot->finished or
> slot->in_use:
>
>     if (slot->finished && slot->in_use == *slot->finished) BUG("in-use = %d and finished = %d disconnect", slot->in_use, *slot->finished);
>     if (!slot->finished && !slot->in_use) BUG("have !in-use and no finished pointer");
>
> Which never fires, but we would get occurrences of:
>
>     if (!slot->finished && slot->in_use) BUG("have in-use and no finished pointer");
>
> I.e. we can simply drop the field and rely on "slot->in_use" in cases
> where we used "finished" before. The two fields were mirror images of
> each other, and the tri-state nature of "finished" wasn't something we
> relied upon.

This sort of thing always makes me a little nervous, regardless of how carefully it's done. I'm not sure I quite follow the above reasoning, but let's take a look at the code...

Show 10 quoted lines

> diff --git a/http-walker.c b/http-walker.c
> index 910fae539b8..5cc369dea85 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -225,13 +225,9 @@ static void process_alternates_response(void *callback_data)
>  					 alt_req->url->buf);
>  			active_requests++;
>  			slot->in_use = 1;
> -			if (slot->finished != NULL)
> -				(*slot->finished) = 0;

Makes sense; here we set slot->in_use to 1, and would have set slot->finished to 0.

Show 5 quoted lines

>  			if (!start_active_slot(slot)) {
>  				cdata->got_alternates = -1;
>  				slot->in_use = 0;
> -				if (slot->finished != NULL)
> -					(*slot->finished) = 1;

Vice-versa here, OK.

Show 11 quoted lines

> diff --git a/http.c b/http.c
> index 229da4d1488..4507c9ac9c7 100644
> --- a/http.c
> +++ b/http.c
> @@ -197,9 +197,6 @@ static void finish_active_slot(struct active_request_slot *slot)
>  	closedown_active_slot(slot);
>  	curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);
>
> -	if (slot->finished != NULL)
> -		(*slot->finished) = 1;
> -

But this one I don't quite follow. Or, at least, I don't readily see that slot->in_use is necessarily going to be 0 here, or (if it isn't) that we somehow don't care.

Could you walk me through your reasoning on why this particular hunk is OK?

Show 12 quoted lines

> @@ -1327,10 +1323,8 @@ void run_active_slot(struct active_request_slot *slot)
>  	fd_set excfds;
>  	int max_fd;
>  	struct timeval select_timeout;
> -	int finished = 0;
>
> -	slot->finished = &finished;
> -	while (!finished) {
> +	while (slot->in_use) {
>  		step_active_slots();
>
>  		if (slot->in_use) {

This part of the diff looks OK to me; you're just swapping the use of '!finished' with 'slot->in_use', which makes sense *assuming* that they are truly mirror images of each other.

The rest of the diff looks good to me, but I do think we should nail down an answer to the question that I posed earlier in this message first.

Thanks, Taylor