git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jun 16, 2022, 13:02 UTC
Message-ID
<220616.86o7yswyek.gmgdl@evledraar.gmail.com>
In-Reply-To
<xmqq8rpxw6ch.fsf@gitster.g>
On Wed, Jun 15 2022, Junio C Hamano wrote:
Show 32 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> 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.

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).

Previous: Junio C HamanoNext: Junio C Hamano
Message 33 of 38 in “Coverity fixes”
  1. 00/11 Coverity fixesJohannes Schindelin via GitGitGadget, Jun 15, 2022
  2. 01/11 mingw: avoid accessing uninitialized memory in `is_executable()`Johannes Schindelin via GitGitGadget, Jun 15, 2022
  3. Junio C HamanoJun 16, 2022
  4. René ScharfeJun 16, 2022
  5. Junio C HamanoJun 16, 2022
  6. Junio C HamanoJun 16, 2022
  7. 02/11 fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`Johannes Schindelin via GitGitGadget, Jun 15, 2022
  8. Junio C HamanoJun 16, 2022
  9. 03/11 submodule--helper: avoid memory leak in `update_submodule()`Johannes Schindelin via GitGitGadget, Jun 15, 2022
  10. Junio C HamanoJun 16, 2022
  11. Glen ChooJun 16, 2022
  12. 04/11 get_oid_with_context_1(): avoid use-after-freeJohannes Schindelin via GitGitGadget, Jun 15, 2022
  13. Junio C HamanoJun 16, 2022
  14. 05/11 submodule-config: avoid memory leakJohannes Schindelin via GitGitGadget, Jun 15, 2022
  15. Junio C HamanoJun 16, 2022
  16. Glen ChooJun 16, 2022
  17. 06/11 pack-redundant: avoid using uninitialized memoryJohannes Schindelin via GitGitGadget, Jun 15, 2022
  18. Junio C HamanoJun 16, 2022
  19. 07/11 submodule--helper: avoid memory leak when fetching submodulesJohannes Schindelin via GitGitGadget, Jun 15, 2022
  20. Junio C HamanoJun 16, 2022
  21. 08/11 read_index_from(): avoid memory leakJohannes Schindelin via GitGitGadget, Jun 15, 2022
  22. 08/11 read_index_from(): avoid memory leakTom Levy, Jun 17, 2022
  23. 09/11 pack-mtimes: avoid closing a bogus file descriptorJohannes Schindelin via GitGitGadget, Jun 15, 2022
  24. Taylor BlauJun 16, 2022
  25. 10/11 relative_url(): fix incorrect conditionJohannes Schindelin via GitGitGadget, Jun 15, 2022
  26. Junio C HamanoJun 16, 2022
  27. Ævar Arnfjörð BjarmasonJun 16, 2022
  28. Junio C HamanoJun 16, 2022
  29. Junio C HamanoJun 16, 2022
  30. 11/11 bug_fl(): add missing `va_end()` callJohannes Schindelin via GitGitGadget, Jun 15, 2022
  31. Jeff KingJun 16, 2022
  32. Junio C HamanoJun 16, 2022
  33. Ævar Arnfjörð BjarmasonJun 16, 2022
  34. Junio C HamanoJun 16, 2022
  35. Jeff KingJun 16, 2022
  36. bug_fl(): correctly initialize trace2 va_listJeff King, Jun 16, 2022
  37. Junio C HamanoJun 16, 2022
  38. Junio C HamanoJun 16, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.