Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call
Jeff King <peff@peff.net> writes:
Show 26 quoted lines
> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> 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.
Show 33 quoted lines
> 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. :)
>
> -PeffJunio C Hamano <gitster@pobox.com>