[FFmpeg-devel] [PATCH]lavu/mem: Make alloc array functions more similar to av_malloc

[James Almer]

On 4/12/2020 7:26 PM, Carl Eugen Hoyos wrote:
> Am Mo., 13. Apr. 2020 um 00:20 Uhr schrieb James Almer <jamrial at gmail.com>:
>>
>> On 4/12/2020 7:07 PM, Carl Eugen Hoyos wrote:
>>> Am So., 12. Apr. 2020 um 23:58 Uhr schrieb James Almer <jamrial at gmail.com>:
>>>>
>>>> On 4/12/2020 6:53 PM, Carl Eugen Hoyos wrote:
>>>>> Am So., 12. Apr. 2020 um 23:52 Uhr schrieb James Almer <jamrial at gmail.com>:
>>>>>>
>>>>>> On 4/12/2020 5:55 PM, Carl Eugen Hoyos wrote:
>>>>>>> Am So., 12. Apr. 2020 um 22:48 Uhr schrieb James Almer <jamrial at gmail.com>:
>>>>>>>>
>>>>>>>> On 4/11/2020 8:53 PM, Carl Eugen Hoyos wrote:
>>>>>>>>> Am So., 12. Apr. 2020 um 00:44 Uhr schrieb Carl Eugen Hoyos
>>>>>>>>> <ceffmpeg at gmail.com>:
>>>>>>>>>>
>>>>>>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
>>>>>>>>>> <michael at niedermayer.cc>:
>>>>>>>>>>>
>>>>>>>>>>> On Sat, Apr 04, 2020 at 12:46:36AM +0200, Carl Eugen Hoyos wrote:
>>>>>>>>>>>> Hi!
>>>>>>>>>>>>
>>>>>>>>>>>> Attached patch makes the alloc array functions more similar to
>>>>>>>>>>>> av_malloc, depending on max_alloc_size instead of INT_MAX.
>>>>>>>>>>>>
>>>>>>>>>>>> Allows a work-around for ticket #7140
>>>>>>>>>>>>
>>>>>>>>>>>> Please comment, Carl Eugen
>>>>>>>>>>>
>>>>>>>>>>>>  mem.c |    8 ++++----
>>>>>>>>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>>>>>> 507531ed6f0932834d005bc1dd7d18e762f158b2  0001-lavu-mem-Make-alloc-array-functions-more-similar-to-.patch
>>>>>>>>>>>> From 7ae240a9f7885130251031aba5d0764b11947fec Mon Sep 17 00:00:00 2001
>>>>>>>>>>>> From: Carl Eugen Hoyos <ceffmpeg at gmail.com>
>>>>>>>>>>>> Date: Sat, 4 Apr 2020 00:37:03 +0200
>>>>>>>>>>>> Subject: [PATCH] lavu/mem: Make alloc array functions more similar to
>>>>>>>>>>>>  av_malloc().
>>>>>>>>>>>>
>>>>>>>>>>>> Do not limit the array allocation functions to allocations of INT_MAX,
>>>>>>>>>>>> instead depend on max_alloc_size like av_malloc().
>>>>>>>>>>>>
>>>>>>>>>>>> Allows a workaround for ticket #7140.
>>>>>>>>>>>> ---
>>>>>>>>>>>>  libavutil/mem.c | 8 ++++----
>>>>>>>>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> av_size_mult() may be faster
>>>>>>>>>>
>>>>>>>>>> New patch attached.
>>>>>>>>>
>>>>>>>>> And an actually working variant.
>>>>>>>>>
>>>>>>>>> Please comment, Carl Eugen
>>>>>>>>
>>>>>>>>> From 643c501d6698d7d17e47a9f907165649f1446fa6 Mon Sep 17 00:00:00 2001
>>>>>>>>> From: Carl Eugen Hoyos <ceffmpeg at gmail.com>
>>>>>>>>> Date: Sun, 12 Apr 2020 00:36:30 +0200
>>>>>>>>> Subject: [PATCH] lavu/mem: Make other alloc functions more similar to av_malloc().
>>>>>>>>>
>>>>>>>>> Do not limit the array allocation functions and av_calloc() to allocations
>>>>>>>>> of INT_MAX, instead depend on max_alloc_size like av_malloc().
>>>>>>>>>
>>>>>>>>> Allows a workaround for ticket #7140.
>>>>>>>>> ---
>>>>>>>>>  libavutil/mem.c | 20 ++++++++++++--------
>>>>>>>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>>>>>>>>> index 88fe09b179..e044374c62 100644
>>>>>>>>> --- a/libavutil/mem.c
>>>>>>>>> +++ b/libavutil/mem.c
>>>>>>>>> @@ -183,23 +183,26 @@ int av_reallocp(void *ptr, size_t size)
>>>>>>>>>
>>>>>>>>>  void *av_malloc_array(size_t nmemb, size_t size)
>>>>>>>>>  {
>>>>>>>>> -    if (!size || nmemb >= INT_MAX / size)
>>>>>>>>> +    size_t result;
>>>>>>>>> +    if (av_size_mult(nmemb, size, &result) < 0)
>>>>>>>>>          return NULL;
>>>>>>>>> -    return av_malloc(nmemb * size);
>>>>>>>>> +    return av_malloc(result);
>>>>>>>>
>>>>>>>> If I'm reading this right, when size is 0, instead of NULL this will now
>>>>>>>> return av_malloc(0), which looks like it may end up being a pointer to a
>>>>>>>> 1 byte big buffer. Is that intended?
>>>>>>>>
>>>>>>>> The previous version you sent apparently considered that scenario.
>>>>>>>
>>>>>>> But it did not pass fate because the behaviour before the patch
>>>>>>> was not to return NULL for alloc(0).
>>>>>>
>>>>>> Before this patch it would return NULL when size was 0 and alloc(0) when
>>>>>> nmemb was 0. Now it will return alloc(0) when either of the two
>>>>>> arguments is 0.
>>>>>>
>>>>>> The check should be (!size || av_size_mult(nmemb, size, &result) < 0) or
>>>>>> similar instead, if we want to keep the original behavior.
>>>>>
>>>>> How did the original behaviour make any sense?
>>>>
>>>> Not saying it made sense, i'm saying we changed that behavior when the
>>>> patch, at least based on the description, only tried to replace the
>>>> INT_MAX limit with max_alloc_size.
>>>>
>>>> If making size 0 return malloc(0) was intended, or ultimately preferred,
>>>> then I'll not oppose to it.
>>>
>>> To me, the old behaviour (returning NULL for some argument being 0
>>> but not the other) made less sense than the new behaviour (not
>>> special-casing 0 for any argument).
>>> The fact that returning NULL broke fate surprised me but I failed
>>> to find the reason.
>>
>> I'm fairly sure your first patch failed because it made one of these
>> functions return NULL for an nmemb == 0 case in some test, when it was
>> not expected to.
> 
> Of course.
> 
> But I failed to find the calling function.
> 
>> Also, if you look at the doxy for these functions, they mention how they
>> should behave depending on if size or nmemb are 0. Most of these don't
>> seem to be honoring their documentation.
> 
> There was never anything free'd, no?

Only av_reallocp(), it seems, and still does. So either the rest should
be adapted to follow their documentation, or the documentation adapted
to the actual behavior.