Am 04.07.21 um 11:01 schrieb Jeff King:
> 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é
