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

From: Jeff King <peff@peff.net>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call
Date: Thu, 16 Jun 2022 00:53:15 -0400	[thread overview]
Message-ID: <Yqq3O5hykBecoVKQ@coredump.intra.peff.net> (raw)
In-Reply-To: <d674aefa78bdb6d255e40af2f308abf8a87a593a.1655336146.git.gitgitgadget@gmail.com>

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.

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
