From: Taylor Blau <me@ttaylorr.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0
Date: Wed, 26 Jan 2022 16:59:32 -0500 [thread overview]
Message-ID: <YfHERK0RUwVjM963@nand.local> (raw)
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:
> 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...
> 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.
> if (!start_active_slot(slot)) {
> cdata->got_alternates = -1;
> slot->in_use = 0;
> - if (slot->finished != NULL)
> - (*slot->finished) = 1;
Vice-versa here, OK.
> 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?
> @@ -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
next prev parent reply other threads:[~2022-01-26 21:59 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-26 21:30 [PATCH] http API: fix dangling pointer issue noted by GCC 12.0 Ævar Arnfjörð Bjarmason
2022-01-26 21:59 ` Taylor Blau [this message]
2022-01-27 0:50 ` Junio C Hamano
2022-01-27 0:57 ` Junio C Hamano
2022-01-27 3:45 ` Ævar Arnfjörð Bjarmason
2022-01-27 18:23 ` Junio C Hamano
2022-02-25 9:09 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2022-02-25 22:58 ` Junio C Hamano
2022-02-26 18:01 ` Taylor Blau
2022-03-25 14:34 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2022-03-25 18:11 ` Taylor Blau
2022-03-26 0:13 ` Junio C Hamano
2022-04-14 15:27 ` Ævar Arnfjörð Bjarmason
2022-04-14 17:04 ` Junio C Hamano
2022-04-15 13:30 ` Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YfHERK0RUwVjM963@nand.local \
--to=me@ttaylorr.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).