patchhttp API: fix dangling pointer issue noted by GCC 12.0

The pre-release GCC 12.0 development branch has a new warning about dangling pointers in -Wall:

    http.c: In function ‘run_active_slot’:
    http.c:1332:24: error: storing the address of local variable ‘finished’ in ‘*slot.finished’ [-Werror=dangling-pointer=]
     1332 |         slot->finished = &finished;
          |         ~~~~~~~~~~~~~~~^~~~~~~~~~~
    http.c:1330:13: note: ‘finished’ declared here
     1330 |         int finished = 0;
          |             ^~~~~~~~

This is on a locally built "gcc (GCC) 12.0.1 20220120 (experimental)", built from gcc.git's 8bc700f4c3f (Enhance vec_pack_trunc for integral mode mask., 2022-01-17).

To fix this I first simply made the member "int finished", i.e. removing the pointer indirection. It turns out that nothing cared about the state of it being a NULL pointer v.s. "*ptr == 0".

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A look at GCC's release history suggests that GCC 12.0 will be released sometime in the next 3-6 months: https://gcc.gnu.org/releases.html

By addressing this new -Wall warning early we can get ahead of the annoying compilation error we'll run into with DEVELOPER=1 once it's released.

 http-walker.c | 4 ----
 http.c        | 8 +-------
 http.h        | 1 -
 3 files changed, 1 insertion(+), 12 deletions(-)

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

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...

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

Vice-versa here, OK.

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?

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I am puzzled by this error. The assignment is the only one that assigns a real pointer to the .finished member, and until finish_active_slot() is called on the slot, the loop would not leave. I would understand the error if slot->finished is used after the function returns to the caller, but I do not think it is the case.

The original code is also curious in that finished is a pointer to somewhere else other than a member that records a yes/no itself.

Sorry, but "instrument and ran test" does not give much confidence.

What was the original problem the "finished" member wanted to solve, and why was the problem unsolveable by simply looking at the in_use member that already existed back then? Isn't the issue very much timing dependent, influenced by the way the HTTP server we are interacting with behaves? Before baa7b67d091acf, the loop in run_active_slot() checked _exactly_ the same "slot->in_use" field, which the above analysis claims to be the mirror image of "finished", so I find that ...

... this reversion of baa7b67d091acf is not very well explained.

The only way the separation could make a difference is while step_active_slots(), the current slot is completed, our local "finished" gets marked as such thanks to the pointer-ness of the finished member, and then another pending request is started reusing the same slot object (properly initialized for that new request). In such a case, the while loop we want to see exit will see that slot->in_use member is _still_ true, even though it is true only because it is now about a separate and unrelated request that is still waiting for completion, and the original request the caller is waiting for has already finished.

And if that is the kind of racy interaction between requests the original code wanted to fix, then I am not sure how this updated code deals with the issue in a different way. Can we safely tell if the original request, held in slot when this function was entered, has completed by looking at slot->in_use member? When the while() loop sees that slot->in_use member is true, how do we know if it is true because nothing has been done to the request yet, or we've completed the original request but another request in queue has replacd the same slot object and the slot is now in-use again?

Junio C Hamano <gitster@pobox.com> writes:

IOW, I am wondering if this is a mistaken compiler that needs to be told not to raise a false warning.

If the motivation behind the original "do not get fooled by a reused slot still working on somebody else's request---instead return when our request is done" was indeed what I speculated, then the pointer slot->finished when we leave this function should not matter to anybody. Would the following patch make the compiler realize that we never smuggle a local variable's address out of this function via a pointer in the structure?

 http.c | 3 +++
 1 file changed, 3 insertions(+)

On Wed, Jan 26 2022, Junio C Hamano wrote:

Yes, that does quiet it. The GCC warning is specifically about pointers
that survive the exit of the function. From the commit that added it to
gcc.git:
    
    +      /* The use is one of a dangling pointer if a clobber of the variable
    +        [the pointer points to] has not been found before the function exit
    +        point.  */
    

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Thanks for a quick testing.

The real question is if this breaks anything. By definition, if this changes the behaviour, it is an indication that the code has been somehow depending on having a pointer to a stackframe that has already gone out of scope, which is a more serious problem. I am wondering if we need to dig further to find it out, and if so, how.

In any case, it looks like a more correct fix to address the "early GCC 12" problem, than reverting baa7b67d (HTTP slot reuse fixes, 2006-03-10), at least to me.

Unless we devise another way to address the "slot reuse" issue, or we come up with an explanation that the "slot reuse" issue is no longer possible in todays code, that is.

Thanks.

The pre-release GCC 12.0 development branch has a new warning about dangling pointers in -Wall:

    http.c: In function ‘run_active_slot’:
    http.c:1332:24: error: storing the address of local variable ‘finished’ in ‘*slot.finished’ [-Werror=dangling-pointer=]
     1332 |         slot->finished = &finished;
          |         ~~~~~~~~~~~~~~~^~~~~~~~~~~
    http.c:1330:13: note: ‘finished’ declared here
     1330 |         int finished = 0;
          |             ^~~~~~~~

This is on a locally built "gcc (GCC) 12.0.1 20220120 (experimental)", built from gcc.git's 8bc700f4c3f (Enhance vec_pack_trunc for integral mode mask., 2022-01-17).

The GCC warning is specifically about pointers that survive the exit of the function. See a comment added to "pass_waccess::use_after_inval_p" in the GCC commit that added the warning, or:

    /* The use is one of a dangling pointer if a clobber of the variable
      [the pointer points to] has not been found before the function exit
      point.  */
    [...]

There's a few possible ways to fix this, but the simplest is to assign NULL to "slot->finished" at the end of run_active_slot(), it's the only caller that ever assigns non-NULL to it. It was suggested[2] to guard that with "if (slot->finished == &finished)", but that'll still trigger the warning.

1. https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9d6a0f388eb048f8d87f47af78f07b5ce513bfe6
2. https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A much simpler fix for a warning new in the GCC v12 pre-release.

Range-diff against v1:
1:  1cec367e805 ! 1:  777838267a5 http API: fix dangling pointer issue noted by GCC 12.0
    @@ Commit message
         built from gcc.git's 8bc700f4c3f (Enhance vec_pack_trunc for integral
         mode mask., 2022-01-17).
     
    -    To fix this I first simply made the member "int finished",
    -    i.e. removing the pointer indirection. It turns out that nothing cared
    -    about the state of it being a NULL pointer v.s. "*ptr == 0".
    +    The GCC warning is specifically about pointers that survive the exit
    +    of the function. See a comment added to
    +    "pass_waccess::use_after_inval_p" in the GCC commit that added the
    +    warning, or:
     
    -    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:
    +        /* The use is one of a dangling pointer if a clobber of the variable
    +          [the pointer points to] has not been found before the function exit
    +          point.  */
    +        [...]
     
    -        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");
    +    There's a few possible ways to fix this, but the simplest is to assign
    +    NULL to "slot->finished" at the end of run_active_slot(), it's the
    +    only caller that ever assigns non-NULL to it. It was suggested[2] to
    +    guard that with "if (slot->finished == &finished)", but that'll still
    +    trigger the warning.
     
    -    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.
    +    1. https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9d6a0f388eb048f8d87f47af78f07b5ce513bfe6
    +    2. https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## http-walker.c ##
    -@@ http-walker.c: 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;
    - 		}
    -
      ## http.c ##
    -@@ http.c: 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;
    --
    - 	/* Store slot results so they can be read after the slot is reused */
    - 	if (slot->results != NULL) {
    - 		slot->results->curl_result = slot->curl_result;
    -@@ http.c: 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);
     @@ http.c: 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();
    + 			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
    + 		}
    + 	}
    ++	slot->finished = NULL;
    + }
      
    - 		if (slot->in_use) {
    -
    - ## http.h ##
    -@@ http.h: struct active_request_slot {
    - 	int in_use;
    - 	CURLcode curl_result;
    - 	long http_code;
    --	int *finished;
    - 	struct slot_results *results;
    - 	void *callback_data;
    - 	void (*callback_func)(void *data);
    + static void release_active_slot(struct active_request_slot *slot)

 http.c | 1 +
 1 file changed, 1 insertion(+)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Does that mean we can clobber the finished member of a slot that was in use, not the one we prepared, because we do not make sure we do not clobber slot->finished that other people set up?

I think this change is safe, but it's been quite a while since I played with dumb HTTP walker the last time, so I no longer trust my own reading of this code X-<.

Will queue, but it would be nice if GCC can get fixed before we have to advance this to 'next' and below

Thanks. .

On Fri, Feb 25, 2022 at 10:09:11AM +0100, Ævar Arnfjörð Bjarmason wrote:

I'm not quite sure that I follow this. This isn't the only spot that assigns non-NULL to "slot->finished", see the assignments in http-walker.c:process_alternates_response() and http.c:finish_active_slot().

But even if it were, I'm not sure how this being the only spot that *writes* non-NULL matters from a reader's perspective.

Looking more at process_alternates_response(), it really looks like this variable wants to hold a tri-state value. I wonder if it would be clearer to replace the NULL/(pointer to) 0/(pointer to) 1 with a UNKNOWN/TRUE/FALSE enum.

Thanks, Taylor

The pre-release GCC 12.0 development branch has a new warning about dangling pointers in -Wall:

    http.c: In function ‘run_active_slot’:
    http.c:1332:24: error: storing the address of local variable ‘finished’ in ‘*slot.finished’ [-Werror=dangling-pointer=]
     1332 |         slot->finished = &finished;
          |         ~~~~~~~~~~~~~~~^~~~~~~~~~~
    http.c:1330:13: note: ‘finished’ declared here
     1330 |         int finished = 0;
          |             ^~~~~~~~

This is on a locally built "gcc (GCC) 12.0.1 20220120 (experimental)", built from gcc.git's 8bc700f4c3f (Enhance vec_pack_trunc for integral mode mask., 2022-01-17).

The GCC warning is specifically about pointers that survive the exit of the function. See a comment added to "pass_waccess::use_after_inval_p" in the GCC commit that added the warning, or:

    /* The use is one of a dangling pointer if a clobber of the variable
      [the pointer points to] has not been found before the function exit
      point.  */
    [...]

There's a few possible ways to fix this, but the simplest is to assign NULL to "slot->finished" at the end of run_active_slot().

This isn't the only caller that assigns to "slot->finished", see see the assignments in http-walker.c:process_alternates_response() and http.c:finish_active_slot().

But those assignments are both to the pointer to our local variable here, so this fix is correct. The only way that code in http-walker.c could have done its assignments is to the pointer to this specific variable.

It was suggested[2] to guard that with "if (slot->finished == &finished)", but that'll still trigger the warning.

1. https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9d6a0f388eb048f8d87f47af78f07b5ce513bfe6
2. https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This should clarify the feedback on v2, sorry about the very late re-roll.

I.e. I *meant* in v2 that it's the only assignment to slot->finished itself is here, the other assignments in http-walker.c are to the pointer to our variable here.

Range-diff against v2:
1:  777838267a5 ! 1:  69190804c67 http API: fix dangling pointer issue noted by GCC 12.0
    @@ Commit message
             [...]
     
         There's a few possible ways to fix this, but the simplest is to assign
    -    NULL to "slot->finished" at the end of run_active_slot(), it's the
    -    only caller that ever assigns non-NULL to it. It was suggested[2] to
    -    guard that with "if (slot->finished == &finished)", but that'll still
    -    trigger the warning.
    +    NULL to "slot->finished" at the end of run_active_slot().
    +
    +    This isn't the only caller that assigns to "slot->finished", see see
    +    the assignments in http-walker.c:process_alternates_response() and
    +    http.c:finish_active_slot().
    +
    +    But those assignments are both to the pointer to our local variable
    +    here, so this fix is correct. The only way that code in http-walker.c
    +    could have done its assignments is to the pointer to this specific
    +    variable.
    +
    +    It was suggested[2] to guard that with "if (slot->finished ==
    +    &finished)", but that'll still trigger the warning.
     
         1. https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9d6a0f388eb048f8d87f47af78f07b5ce513bfe6
         2. https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/

 http.c | 1 +
 1 file changed, 1 insertion(+)

On Fri, Mar 25, 2022 at 03:34:49PM +0100, Ævar Arnfjörð Bjarmason wrote:

s/see see/see ?

Got it; this is the key piece that I was missing in my earlier review. Sorry about that, this looks completely safe to me now.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

It does not exactly look safe to me, though.

With a bit of rewrite, here is what I'd queue for now. I really hope that the pre-release compiler will be fixed soon so that we do not have to worry about this patch.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Fri, 25 Mar 2022 15:34:49 +0100

The pre-release GCC 12.0 development branch has a new warning about dangling pointers in -Wall:

    http.c: In function ‘run_active_slot’:
    http.c:1332:24: error: storing the address of local variable ‘finished’ in ‘*slot.finished’ [-Werror=dangling-pointer=]
     1332 |         slot->finished = &finished;
          |         ~~~~~~~~~~~~~~~^~~~~~~~~~~
    http.c:1330:13: note: ‘finished’ declared here
     1330 |         int finished = 0;
          |             ^~~~~~~~

This is on a locally built "gcc (GCC) 12.0.1 20220120 (experimental)", built from gcc.git's 8bc700f4c3f (Enhance vec_pack_trunc for integral mode mask., 2022-01-17).

The GCC warning is specifically about pointers that survive the exit of the function. See a comment added to "pass_waccess::use_after_inval_p" in the GCC commit that added the warning, or:

    /* The use is one of a dangling pointer if a clobber of the variable
      [the pointer points to] has not been found before the function exit
      point.  */
    [...]

There's a few possible ways to fix this, but the simplest is to assign NULL to "slot->finished" at the end of run_active_slot().

It was suggested[2] to guard that with "if (slot->finished == &finished)", but that'll still trigger the bogus warning from the compiler.

This is the only place that assigns to "slot->finished"; other mention of slot->finished in http-walker.c:process_alternates_response() and http.c:finish_active_slot() are assignments through the pointer, and not moving where the pointer points at.

As long as the same slot is never passed to run_active_slot() recursively, clearing the member unconditionally when the control leaves the function should not break the code. Knock wood, as nobody seems to have made sure if that is the case.

We could add

	if (slot->finished)
		BUG("the uncoditional clearing at the end is wrong");

early in run_active_slot(), before we assign the pointer to our on-stack variable to this field, to give us such a guarantee, though.

1. https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9d6a0f388eb048f8d87f47af78f07b5ce513bfe6
2. https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c | 1 +
 1 file changed, 1 insertion(+)

On Fri, Mar 25 2022, Junio C Hamano wrote:

Late reply, sorry.

I don't think the compiler is broken in this case.

Having spelunked in the GCC docs, source, commits involved & in their bugtracker I don't think they'd consider this broken. It's working as designed.

Now, of course as with any new compiler warning you might argue that it's too overzealous, but in this case it's included it a -Wall in GCC 12.0.

The warning is specifically about the local pointer being stored in a structure that survives the exit of function, and therefore if you were to dereference that pointer after the function exists the results are undefined.

Now, we happen to be able to carefully read the code in this case and reason that we won't be looking at it except in the code that's called within this function, so in practice we're OK.

But we'd have a bug sneak up on us if that wasn't the case, so it's safer to NULL this out, which is exactly what the new GCC warning is concerning itself with.

I.e. it's not promising to narrow itself to only those cases where GCC can know for absolute certain that it's a *practical* issue.

Which, basically would be asking for it to do as much work to emit it as it has to do on -fanalyzer, which makes compilation of git.git take somewhere in the mid-range of double digit minutes for me, instead of ~20s.

So I'd prefer:

 1. Adjust for release etc., but that what I submitted in
    https://lore.kernel.org/git/patch-v3-1.1-69190804c67-20220325T143322Z-avarab@gmail.com/
    go in as-is.

 2. If you're convinced this is a compiler bug could you please either
    drop the current version, or rewrite the commit envelope so that
    you're the author?

    I very much appreciate when you do local commit fix-ups for
    rewording, typos, or even rewriting something I did for the better.

    But in this case it's got my name on the envelope & has been
    reworded to say pretty much the exact opposite of what I
    think/believe about this warning.

    Which isn't something wrong or not permitted by the
    license/SOB/whatever.

    I'd just find it confusing when I'll dig this up at some point in
    the future to find my name on it, read the explanation, and then be
    perplexed why I ever thought that about this particular warning
    ... :)

Thanks.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Whatever. I do not care if this is a broken or wai from GCC's point of view.

I care more about the correct operation of the production code of ours, than a workaround to squelch a warning, overzealous or not, by a compiler, and if it is not clear that the workaround is obviously free of negative side effect, we do not want to deliberately introduce potential breakage in our code base just for that.

And I still do not see how it is safe to unconditionally clearing the pointer in the slot instance to NULL. It was asked late January in https://lore.kernel.org/git/xmqqv8y52g3a.fsf@gitster.g/

In other words, what we should have been spelunking is *not* in the GCC docs and code, but the http codepath in our code that depends on the slot not being cleared when we didn't set up the pointer in the current recursion of that function. With a clear description on how this change is safe, with a clear understanding on why the pointer member "finished" was added in the first place to avoid the same mistake as an earlier round of this patch [*1*], it would become an acceptable patch, with or without GCC warning.

Namely, the finding in this part of a review comment [*2*]

    The only way the separation could make a difference is while
    step_active_slots(), the current slot is completed, our local
    "finished" gets marked as such thanks to the pointer-ness of the
    finished member, and then another pending request is started
    reusing the same slot object (properly initialized for that new
    request).  In such a case, the while loop we want to see exit
    will see that slot->in_use member is _still_ true, even though
    it is true only because it is now about a separate and unrelated
    request that is still waiting for completion, and the original
    request the caller is waiting for has already finished.

that was made to explain why the pointer member is there, and a possible case that the code before the introduction of the pointer would misbehave and today's code would behave better, worries me the most, as unconditionally assigning NULL there like this patch does without any guard would mean that we are breaking the code exactly in such a case, I would think.

In short, I do not care who takes the credit, I care more about the correctness of the code than a warning by a version of a compiler, I do not care at all if the compiler writers considers the warning a bug, and I worry that the change proposed, while it may certainly squelch the bug, may break the code that has been working happily, and I haven't seen a clear explanation why it is not the case.

As long as the same slot is never passed to run_active_slot() recursively, clearing the member unconditionally when the control leaves the function should not break the code. Nobody seems to have explained how it is the case.

[References]

*1* https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/ *2* https://lore.kernel.org/git/xmqqczkengsg.fsf@gitster.g/

On Thu, Apr 14 2022, Junio C Hamano wrote:

The v3 re-roll at https://lore.kernel.org/git/patch-v3-1.1-69190804c67-20220325T143322Z-avarab@gmail.com/ (see range diff) was intended to address both your question there & Taylor's at https://lore.kernel.org/git/YhprAb1f1WYIktCV@nand.local/.

I think that accurately describes a logic error in the v1 of this patch, i.e. we can't remove the "finished" member, but per the v3 explanation I believe (re-)setting it to NULL is safe.

I tried to explain that in the v3, but that was part of what you edited/amended in your applied version of it.

I don't know how to answer your concerns/questions other than as I've already done there.