Re: [PATCH] http.c: clear the 'finished' member once we are done with it

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 23, 2022, 21:58 UTC
Message-ID: <nycvar.QRO.7.76.6.2205232248360.352@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqczgqjr8y.fsf_-_@gitster.g>

Hi Junio,

On Fri, 6 May 2022, Junio C Hamano wrote:

Show 38 quoted lines

> In http.c, the run_active_slot() function allows the given "slot" to
> make progress by calling step_active_slots() in a loop repeatedly,
> and the loop is not left until the request held in the slot
> completes.
>
> Ages ago, we used to use the slot->in_use member to get out of the
> loop, which misbehaved when the request in "slot" completes (at
> which time, the result of the request is copied away from the slot,
> and the in_use member is cleared, making the slot ready to be
> reused), and the "slot" gets reused to service a different request
> (at which time, the "slot" becomes in_use again, even though it is
> for a different request).  The loop terminating condition mistakenly
> thought that the original request has yet to be completed.
>
> Today's code, after baa7b67d (HTTP slot reuse fixes, 2006-03-10)
> fixed this issue, uses a separate "slot->finished" member that is
> set in run_active_slot() to point to an on-stack variable, and the
> code that completes the request in finish_active_slot() clears the
> on-stack variable via the pointer to signal that the particular
> request held by the slot has completed.  It also clears the in_use
> member (as before that fix), so that the slot itself can safely be
> reused for an unrelated request.
>
> One thing that is not quite clean in this arrangement is that,
> unless the slot gets reused, at which point the finished member is
> reset to NULL, the member keeps the value of &finished, which
> becomes a dangling pointer into the stack when run_active_slot()
> returns.  Clear the finished member before the control leaves the
> function, but make sure to limit it to the case where the pointer
> still points at the on-stack variable of ours (the pointer may be
> set to point at the on-stack variable of somebody else after the
> slot gets reused, in which case we do not want to touch it).
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * So, this has been sitting in my pile of random patches for a few
>    weeks.

I stumbled over the need for this while investigating the build failures caused by upgrading Git for Windows' SDK's GCC to v12.x.

Show 11 quoted lines

> diff --git a/http.c b/http.c
> index 229da4d148..85437b1980 100644
> --- a/http.c
> +++ b/http.c
> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)
>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>  		}
>  	}
> +
> +	if (slot->finished == &finished)
> +		slot->finished = NULL;

First of all, I suspect that https://github.com/git/git/blob/v2.36.1/http.c#L1207 makes sure that GCC's complaint is not actually accurate: we always re-set `finished` to `NULL` when getting an unused slot, so even if there is a left-over dangling pointer, it is not actually used, ever.

But we need something to pacify GCC. Let's look at your patch.

The first thing to note is that this is not _quite_ thread-safe: between checking the condition `slot->finished == &finished` and assigning `slot->finished`, another thread could potentially have noticed that the slot is not in use and overwritten the `finished` attribute, which would then be set to `NULL` in this thread, in which case _that other_ thread's `while (!finished)` loop would become an infinite loop.

Having said that, the time window is really narrow.

Besides, I suspect that we _already_ have an equivalent "offender" in https://github.com/git/git/blob/v2.36.1/http.c#L1336: we look at `in_use` there, assuming that it is either `1` if "our" request is still active, and otherwise it is `0`. However, it might have turned to `0` _and_ to `1` again in the meantime (but the `in_use` would now refer to _another_ request).

I am not quite sure how correct my reading of the situation is, so please double-check my analysis.

If that analysis is correct, I would expect the correct solution to turn `finished` into an attribute of the slot, and change its role to be a flag that this slot is spoken for and cannot be re-used quite yet even if it is not currently in use.

Something like this:

-- snip --

diff --git a/http-walker.c b/http-walker.c
index 910fae539b89..5cc369dea853 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;
 			if (!start_active_slot(slot)) {
 				cdata->got_alternates = -1;
 				slot->in_use = 0;
-				if (slot->finished != NULL)
-					(*slot->finished) = 1;
 			}
 			return;
 		}
diff --git a/http.c b/http.c
index b08795715f8a..2d125132fb90 100644
--- a/http.c
+++ b/http.c
@@ -205,8 +205,7 @@ 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;
+	slot->in_use = 0;

 	/* Store slot results so they can be read after the slot is reused */
 	if (slot->results != NULL) {
@@ -1212,13 +1211,14 @@ struct active_request_slot *get_active_slot(void)
 			process_curl_messages();
 	}

-	while (slot != NULL && slot->in_use)
+	while (slot != NULL && (slot->in_use || slot->reserved_for_use))
 		slot = slot->next;

 	if (slot == NULL) {
 		newslot = xmalloc(sizeof(*newslot));
 		newslot->curl = NULL;
 		newslot->in_use = 0;
+		newslot->reserved_for_use = 0;
 		newslot->next = NULL;

 		slot = active_queue_head;
@@ -1240,7 +1240,6 @@ struct active_request_slot *get_active_slot(void)
 	active_requests++;
 	slot->in_use = 1;
 	slot->results = NULL;
-	slot->finished = NULL;
 	slot->callback_data = NULL;
 	slot->callback_func = NULL;
 	curl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);
@@ -1332,7 +1331,7 @@ void fill_active_slots(void)
 	}

 	while (slot != NULL) {
-		if (!slot->in_use && slot->curl != NULL
+		if (!slot->in_use && !slot->reserved_for_use && slot->curl
 			&& curl_session_count > min_curl_sessions) {
 			curl_easy_cleanup(slot->curl);
 			slot->curl = NULL;
@@ -1363,10 +1362,9 @@ 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) {
+	slot->reserved_for_use = 1;
+	while (slot->in_use) {
 		step_active_slots();

 		if (slot->in_use) {
@@ -1403,6 +1401,7 @@ void run_active_slot(struct active_request_slot *slot)
 			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
 		}
 	}
+	slot->reserved_for_use = 0;
 }

 static void release_active_slot(struct active_request_slot *slot)
diff --git a/http.h b/http.h
index df1590e53a45..3b2f6da570cd 100644
--- a/http.h
+++ b/http.h
@@ -22,9 +22,9 @@ struct slot_results {
 struct active_request_slot {
 	CURL *curl;
 	int in_use;
+	int reserved_for_use;
 	CURLcode curl_result;
 	long http_code;
-	int *finished;
 	struct slot_results *results;
 	void *callback_data;
 	void (*callback_func)(void *data);
-- snap --

I integrated this into a local branch that fixes the build with GCC v12.x
(required so that our CI/PR builds work again after Git for Windows' SDK
upgraded its GCC) and plan on contributing these patches in a bit.

Ciao,
Dscho

>  }
>
>  static void release_active_slot(struct active_request_slot *slot)
> --
> 2.36.1-200-gf89ea983ca
>
>
>