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

Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()

From
PWPhillip Wood <phillip.wood@talktalk.net>
Date
Apr 15, 2022, 14:30 UTC
Message-ID
<23138e2b-436a-0990-cefc-0662674373c3@talktalk.net>
In-Reply-To
<220415.86fsmebgds.gmgdl@evledraar.gmail.com>
Hi Ævar and René
On 15/04/2022 14:53, Ævar Arnfjörð Bjarmason wrote:
Show 23 quoted lines
> 
> On Fri, Apr 15 2022, René Scharfe wrote:
> 
>> Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:
>>> Return NULL instead of possibly feeding a NULL to memset() in
>>> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:
>>>
>>> 	reftable/publicbasics.c: In function ‘reftable_calloc’:
>>> 	reftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]
>>> 	   43 |         memset(p, 0, sz);
>>> 	      |         ^~~~~~~~~~~~~~~~
>>> 	[...]
>>>
>>> This bug has been with us ever since this code was added in
>>> ef8a6c62687 (reftable: utility functions, 2021-10-07).
>>
>> Not sure it's a bug, or limited to this specific location.  AFAICS it's
>> more a general lack of handling of allocation failures in the reftable
>> code.  Perhaps it was a conscious decision to let the system deal with
>> them via segfaults?
> 
> I think it's just buggy, it tries to deal with malloc returning NULL in
> other contexts.
Does it? I just quickly scanned the output of
git grep -e 'reftable_[mc]alloc' -e 'reftable_realloc' -A2 origin/master
and I didn't see a single NULL check
Show 7 quoted lines
>> When the code is used by Git eventually it should use xmalloc and
>> xrealloc instead of calling malloc(3) and realloc(3) directly, to
>> handle allocation failures explicitly, and to support GIT_ALLOC_LIMIT.
>> This will calm down the analyzer and extend the safety to the rest of
>> the reftable code, not just this memset call.
> 
> Yeah, its whole custom malloc handling should go away.

Isn't it there so the same code can be used by libgit2 and other things that let the caller specify a custom allocator?

Best Wishes
Phillip
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 12 of 16 in “reftable: avoid undefined behaviour breaking t0032”
  1. reftable: avoid undefined behaviour breaking t0032Carlo Marcelo Arenas Belón, Apr 15, 2022
  2. Junio C HamanoApr 15, 2022
  3. reftable: avoid undefined behaviour breaking t0032Carlo Marcelo Arenas Belón, Apr 15, 2022
  4. 0/2 reftable: remove poor man's SANITIZE=address, fix a memset() bugÆvar Arnfjörð Bjarmason, Apr 15, 2022
  5. 1/2 reftable: remove the "return_block" abstractionÆvar Arnfjörð Bjarmason, Apr 15, 2022
  6. René ScharfeApr 15, 2022
  7. Han-Wen NienhuysApr 25, 2022
  8. Junio C HamanoApr 25, 2022
  9. 2/2 reftable: don't memset() a NULL from failed malloc()Ævar Arnfjörð Bjarmason, Apr 15, 2022
  10. René ScharfeApr 15, 2022
  11. Ævar Arnfjörð BjarmasonApr 15, 2022
  12. Phillip WoodApr 15, 2022
  13. Ævar Arnfjörð BjarmasonApr 15, 2022
  14. Junio C HamanoApr 15, 2022
  15. Han-Wen NienhuysApr 25, 2022
  16. Han-Wen NienhuysApr 25, 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.