On Wed, Jun 15 2022, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > >> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote: >> >>> From: Johannes Schindelin <johannes.schindelin@xxxxxx> >>> >>> 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).
