[11/11] bug_fl(): add missing `va_end()` call

Commit Message

Johannes Schindelin June 15, 2022, 11:35 p.m. UTC

From: Johannes Schindelin <[e-mail]>

According to the manual:

	Each invocation of va_copy() must be matched by a corresponding
	invocation of va_end() in the  same function.

Note: There is another instance of `va_copy()` in `usage.c` that is
missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
though, because that function either `exit()`s or `abort()`s, anyway.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 usage.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jeff King June 16, 2022, 4:53 a.m. UTC | #1

On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>
> 
> According to the manual:
> 
> 	Each invocation of va_copy() must be matched by a corresponding
> 	invocation of va_end() in the  same function.
> 
> Note: There is another instance of `va_copy()` in `usage.c` that is
> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
> though, because that function either `exit()`s or `abort()`s, anyway.
> 
> Reported by Coverity.

This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal
bug() function to go with BUG(), 2022-06-02). But there's a much worse
bug in the same function. The code introduced by that patch does:

  va_list ap, cp;
  [...]
  va_copy(cp, ap);
  va_start(ap, fmt);

So "cp" is copied from "ap" before we have actually initialized "ap".
It's surprising that this works at all. The two lines should be flipped.

IMHO, since we're in the actual varargs function itself, it would be
simpler to just bracket each use with start/end, rather than copying,
like:

diff --git a/usage.c b/usage.c
index 79900d0287..56e29d6cd6 100644
--- a/usage.c
+++ b/usage.c
@@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 int bug_called_must_BUG;
 void bug_fl(const char *file, int line, const char *fmt, ...)
 {
-	va_list ap, cp;
+	va_list ap;
 
 	bug_called_must_BUG = 1;
 
-	va_copy(cp, ap);
 	va_start(ap, fmt);
 	BUG_vfl_common(file, line, fmt, ap);
 	va_end(ap);
-	trace2_cmd_error_va(fmt, cp);
+
+	va_start(ap, fmt);
+	trace2_cmd_error_va(fmt, ap);
+	va_end(ap);
 }
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS

but I am happy with any solution that is correct. :)

-Peff

Junio C Hamano June 16, 2022, 5 a.m. UTC | #2

Jeff King <[e-mail]> writes:

> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <[e-mail]>
>> 
>> According to the manual:
>> 
>> 	Each invocation of va_copy() must be matched by a corresponding
>> 	invocation of va_end() in the  same function.
>> 
>> Note: There is another instance of `va_copy()` in `usage.c` that is
>> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
>> though, because that function either `exit()`s or `abort()`s, anyway.
>> 
>> Reported by Coverity.
>
> This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal
> bug() function to go with BUG(), 2022-06-02). But there's a much worse
> bug in the same function. The code introduced by that patch does:
>
>   va_list ap, cp;
>   [...]
>   va_copy(cp, ap);
>   va_start(ap, fmt);
>
> So "cp" is copied from "ap" before we have actually initialized "ap".
> It's surprising that this works at all. The two lines should be flipped.

Yes, it is surprising.  Perhaps it is not working at all.

Thanks for an extra set of eyeballs.

> IMHO, since we're in the actual varargs function itself, it would be
> simpler to just bracket each use with start/end, rather than copying,
> like:
>
> diff --git a/usage.c b/usage.c
> index 79900d0287..56e29d6cd6 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
>  int bug_called_must_BUG;
>  void bug_fl(const char *file, int line, const char *fmt, ...)
>  {
> -	va_list ap, cp;
> +	va_list ap;
>  
>  	bug_called_must_BUG = 1;
>  
> -	va_copy(cp, ap);
>  	va_start(ap, fmt);
>  	BUG_vfl_common(file, line, fmt, ap);
>  	va_end(ap);
> -	trace2_cmd_error_va(fmt, cp);
> +
> +	va_start(ap, fmt);
> +	trace2_cmd_error_va(fmt, ap);
> +	va_end(ap);
>  }
>  
>  #ifdef SUPPRESS_ANNOTATED_LEAKS
>
> but I am happy with any solution that is correct. :)
>
> -Peff

Ævar Arnfjörð Bjarmason June 16, 2022, 1:02 p.m. UTC | #3

On Wed, Jun 15 2022, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
>
>> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:
>>
>>> From: Johannes Schindelin <[e-mail]>
>>> 
>>> According to the manual:
>>> 
>>> 	Each invocation of va_copy() must be matched by a corresponding
>>> 	invocation of va_end() in the  same function.
>>> 
>>> Note: There is another instance of `va_copy()` in `usage.c` that is
>>> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
>>> though, because that function either `exit()`s or `abort()`s, anyway.
>>> 
>>> Reported by Coverity.
>>
>> This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal
>> bug() function to go with BUG(), 2022-06-02). But there's a much worse
>> bug in the same function. The code introduced by that patch does:
>>
>>   va_list ap, cp;
>>   [...]
>>   va_copy(cp, ap);
>>   va_start(ap, fmt);
>>
>> So "cp" is copied from "ap" before we have actually initialized "ap".
>> It's surprising that this works at all. The two lines should be flipped.
>
> Yes, it is surprising.  Perhaps it is not working at all.
>
> Thanks for an extra set of eyeballs.

It's "working" now in the sense that we run this code, and even test
that trace2 output specifically, see the tests in 0cc05b044fd (usage.c:
add a non-fatal bug() function to go with BUG(), 2022-06-02).

But obviously that's a bad use of the varargs API, I just don't know how
we've been getting away with it in practice, sorry about that.

The fix Peff's got here LGTM. I can (re)submit it with
format-patch+send-email after giving it a commit message describing the
issue if you'd like, but the change would be the same.

I'm also happy for you to pick up the upthread directly, but I don't see
that you did that already in one of the integration branches (but
perhaps I missed one).

Just let me know, but I didn't want to re-submit this without asking, in
case I'd step on any toes (or submit something in duplicate).

Junio C Hamano June 16, 2022, 6:03 p.m. UTC | #4

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> But obviously that's a bad use of the varargs API, I just don't know how
> we've been getting away with it in practice, sorry about that.

Exactly.  We three all expressed our surprises on why it "works".
Nobody offered an explanation, though, which leaves us in suspense
;-)

> The fix Peff's got here LGTM. I can (re)submit it with
> format-patch+send-email after giving it a commit message describing the
> issue if you'd like, but the change would be the same.

Yup, I think the code change there looks the most sensible.

Thanks, all.

Jeff King June 16, 2022, 7:20 p.m. UTC | #5

On Thu, Jun 16, 2022 at 11:03:25AM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <[e-mail]> writes:
> 
> > But obviously that's a bad use of the varargs API, I just don't know how
> > we've been getting away with it in practice, sorry about that.
> 
> Exactly.  We three all expressed our surprises on why it "works".
> Nobody offered an explanation, though, which leaves us in suspense
> ;-)

Being the curious sort, I ran it in a debugger. And indeed, "cp" is
filled with uninitialized garbage. The reason it works is that the test
calls bug() with a format string that does not contain any placeholders.
And thus our eventual call to vsnprintf() does not ever look at "cp" at
all!

> > The fix Peff's got here LGTM. I can (re)submit it with
> > format-patch+send-email after giving it a commit message describing the
> > issue if you'd like, but the change would be the same.
> 
> Yup, I think the code change there looks the most sensible.

I'll wrap it up with a commit message and modify the test to be more
thorough.

-Peff

Junio C Hamano June 16, 2022, 8:11 p.m. UTC | #6

Jeff King <[e-mail]> writes:

> Being the curious sort, I ran it in a debugger. And indeed, "cp" is
> filled with uninitialized garbage. The reason it works is that the test
> calls bug() with a format string that does not contain any placeholders.
> And thus our eventual call to vsnprintf() does not ever look at "cp" at
> all!

;-)  Thanks.

Patch

diff --git a/usage.c b/usage.c
index 79900d0287f..4849eb75785 100644
--- a/usage.c
+++ b/usage.c
@@ -343,6 +343,7 @@  void bug_fl(const char *file, int line, const char *fmt, ...)
 	BUG_vfl_common(file, line, fmt, ap);
 	va_end(ap);
 	trace2_cmd_error_va(fmt, cp);
+	va_end(cp);
 }
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS