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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call
Date: Thu, 16 Jun 2022 15:20:08 -0400	[thread overview]
Message-ID: <YquCaE+Vw9P/fybU@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqk09gtriq.fsf@gitster.g>

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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