Re: [PATCH] khash: clarify that allocations never fail

From: René Scharfe <l.s.r@web.de>
Date: Jul 4, 2021, 09:41 UTC
Message-ID: <b2c9b072-b2ac-625b-4f0f-9ba1ca3321d7@web.de>
In-Reply-To: <YOF46CU+SSDRE51/@coredump.intra.peff.net>

Am 04.07.21 um 11:01 schrieb Jeff King:

Show 26 quoted lines

> On Sat, Jul 03, 2021 at 02:56:50PM +0200, René Scharfe wrote:
>
>> The following semantic patch finds a leery xmalloc() caller in
>> compat/mmap.c, though:
>>
>> @@
>> expression PTR, SIZE, SIZE2;
>> @@
>> (
>>   PTR = xmalloc(SIZE);
>> |
>>   PTR = xcalloc(SIZE, SIZE2);
>> |
>>   PTR = xrealloc(SIZE);
>> |
>>   ALLOC_ARRAY(PTR, SIZE);
>> |
>>   CALLOC_ARRAY(PTR, SIZE);
>> |
>>   REALLOC_ARRAY(PTR, SIZE);
>> )
>>   if (
>> - PTR == NULL
>> + 0
>>   ) {...}
>

Btw. the found code is:

	start = xmalloc(length);
	if (start == NULL) {
		errno = ENOMEM;
		return MAP_FAILED;
	}

start cannot be NULL, so the check is dead code.

> IMHO that should not be using xmalloc() at all. It is a replacement for
> system mmap, which can fail with ENOMEM, and we should be able to do the
> same. Using xmalloc here is probably losing an opportunity to close
> another pack window to free up memory for a new one.

Do you mean using malloc(3) directly instead of xmalloc() would no longer try to release pack windows? xmalloc() hasn't done that anymore since 9827d4c185 (packfile: drop release_pack_memory(), 2019-08-12).

xmalloc() still brings support for zero-sized allocations on platforms whose malloc(3) doesn't like them, and it enforces GIT_ALLOC_LIMIT. mmap() is supposed give up with EINVAL if the length is 0, so the first feature is not actually helping. And GIT_ALLOC_LIMIT is not documented and only useful for testing, right?

> I doubt it matters that much in practice (most systems are not even
> compiling or using this code, and it would only matter in a tight memory
> situation). But as a general rule, I think compat/ wrappers should
> behave as much like (sensible) system equivalents as possible.

Makes sense.

René