diff mbox series

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

Message ID CAB0OVGrNorBgi+D9FVe_o6g_hiYH0C=qKCcUTxB6iF39cvNyoA@mail.gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,v3] lavu/mem: Make alloc array functions more similar to av_malloc | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Carl Eugen Hoyos April 11, 2020, 11:53 p.m. UTC
Am So., 12. Apr. 2020 um 00:44 Uhr schrieb Carl Eugen Hoyos
<ceffmpeg@gmail.com>:
>
> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
> <michael@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@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

Comments

James Almer April 12, 2020, 8:47 p.m. UTC | #1
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@gmail.com>:
>>
>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
>> <michael@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@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@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.

>  }
>  
>  void *av_mallocz_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_mallocz(nmemb * size);
> +    return av_mallocz(result);
>  }
>  
>  void *av_realloc_array(void *ptr, 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_realloc(ptr, nmemb * size);
> +    return av_realloc(ptr, result);
>  }
>  
>  int av_reallocp_array(void *ptr, size_t nmemb, size_t size)
> @@ -243,9 +246,10 @@ void *av_mallocz(size_t size)
>  
>  void *av_calloc(size_t nmemb, size_t size)
>  {
> -    if (size <= 0 || nmemb >= INT_MAX / size)
> +    size_t result;
> +    if (av_size_mult(nmemb, size, &result) < 0)
>          return NULL;
> -    return av_mallocz(nmemb * size);
> +    return av_mallocz(result);
>  }
>  
>  char *av_strdup(const char *s)
> -- 
> 2.24.1
>
Carl Eugen Hoyos April 12, 2020, 8:55 p.m. UTC | #2
Am So., 12. Apr. 2020 um 22:48 Uhr schrieb James Almer <jamrial@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@gmail.com>:
> >>
> >> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
> >> <michael@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@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@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).

Carl Eugen
James Almer April 12, 2020, 9:51 p.m. UTC | #3
On 4/12/2020 5:55 PM, Carl Eugen Hoyos wrote:
> Am So., 12. Apr. 2020 um 22:48 Uhr schrieb James Almer <jamrial@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@gmail.com>:
>>>>
>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
>>>> <michael@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@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@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.
Carl Eugen Hoyos April 12, 2020, 9:53 p.m. UTC | #4
Am So., 12. Apr. 2020 um 23:52 Uhr schrieb James Almer <jamrial@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@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@gmail.com>:
> >>>>
> >>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
> >>>> <michael@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@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@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?

Carl Eugen
James Almer April 12, 2020, 9:57 p.m. UTC | #5
On 4/12/2020 6:53 PM, Carl Eugen Hoyos wrote:
> Am So., 12. Apr. 2020 um 23:52 Uhr schrieb James Almer <jamrial@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@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@gmail.com>:
>>>>>>
>>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
>>>>>> <michael@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@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@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.
Carl Eugen Hoyos April 12, 2020, 10:07 p.m. UTC | #6
Am So., 12. Apr. 2020 um 23:58 Uhr schrieb James Almer <jamrial@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@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@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@gmail.com>:
> >>>>>>
> >>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
> >>>>>> <michael@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@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@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.

Carl Eugen
Andreas Rheinhardt April 12, 2020, 10:17 p.m. UTC | #7
Carl Eugen Hoyos:
> Am So., 12. Apr. 2020 um 23:58 Uhr schrieb James Almer <jamrial@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@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@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@gmail.com>:
>>>>>>>>
>>>>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
>>>>>>>> <michael@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@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@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.
> 
Which tests failed?

- Andreas
Carl Eugen Hoyos April 12, 2020, 10:18 p.m. UTC | #8
Am Mo., 13. Apr. 2020 um 00:17 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:
>
> Carl Eugen Hoyos:
> > Am So., 12. Apr. 2020 um 23:58 Uhr schrieb James Almer <jamrial@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@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@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@gmail.com>:
> >>>>>>>>
> >>>>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
> >>>>>>>> <michael@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@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@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.
> >
> Which tests failed?

Many (I am not saying that it isn't a single point of failure, but I
didn't find it).

Carl Eugen
James Almer April 12, 2020, 10:20 p.m. UTC | #9
On 4/12/2020 7:07 PM, Carl Eugen Hoyos wrote:
> Am So., 12. Apr. 2020 um 23:58 Uhr schrieb James Almer <jamrial@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@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@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@gmail.com>:
>>>>>>>>
>>>>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
>>>>>>>> <michael@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@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@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.

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.
Carl Eugen Hoyos April 12, 2020, 10:26 p.m. UTC | #10
Am Mo., 13. Apr. 2020 um 00:20 Uhr schrieb James Almer <jamrial@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@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@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@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@gmail.com>:
> >>>>>>>>
> >>>>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
> >>>>>>>> <michael@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@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@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?

Carl Eugen
James Almer April 12, 2020, 10:30 p.m. UTC | #11
On 4/12/2020 7:26 PM, Carl Eugen Hoyos wrote:
> Am Mo., 13. Apr. 2020 um 00:20 Uhr schrieb James Almer <jamrial@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@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@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@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@gmail.com>:
>>>>>>>>>>
>>>>>>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
>>>>>>>>>> <michael@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@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@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.
Andreas Rheinhardt April 12, 2020, 10:47 p.m. UTC | #12
Carl Eugen Hoyos:
> Am Mo., 13. Apr. 2020 um 00:20 Uhr schrieb James Almer <jamrial@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@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@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@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@gmail.com>:
>>>>>>>>>>
>>>>>>>>>> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
>>>>>>>>>> <michael@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@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@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.
> 
Seems to be the MERGE_REF macro in libavfilter/formats.c: A simple check
for a->refcount before performing the reallocation seems to fix most of
the tests.

- Andreas
Anton Khirnov April 14, 2020, 8:57 a.m. UTC | #13
Quoting James Almer (2020-04-12 22:47:55)
> 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@gmail.com>:
> >>
> >> Am So., 5. Apr. 2020 um 14:03 Uhr schrieb Michael Niedermayer
> >> <michael@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@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@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?

IIUC some platforms had problems with malloc(0)
Plus I don't think any valid code should ever malloc zero bytes, so my
suggestion would be for all malloc wrappers to:
- av_assert2(size > 0)
- return NULL otherwise
Nicolas George April 14, 2020, 9:03 a.m. UTC | #14
Anton Khirnov (12020-04-14):
> IIUC some platforms had problems with malloc(0)
> Plus I don't think any valid code should ever malloc zero bytes, so my
> suggestion would be for all malloc wrappers to:
> - av_assert2(size > 0)
> - return NULL otherwise

I think this is a very good idea.

Regards,
diff mbox series

Patch

From 643c501d6698d7d17e47a9f907165649f1446fa6 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@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);
 }
 
 void *av_mallocz_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_mallocz(nmemb * size);
+    return av_mallocz(result);
 }
 
 void *av_realloc_array(void *ptr, 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_realloc(ptr, nmemb * size);
+    return av_realloc(ptr, result);
 }
 
 int av_reallocp_array(void *ptr, size_t nmemb, size_t size)
@@ -243,9 +246,10 @@  void *av_mallocz(size_t size)
 
 void *av_calloc(size_t nmemb, size_t size)
 {
-    if (size <= 0 || nmemb >= INT_MAX / size)
+    size_t result;
+    if (av_size_mult(nmemb, size, &result) < 0)
         return NULL;
-    return av_mallocz(nmemb * size);
+    return av_mallocz(result);
 }
 
 char *av_strdup(const char *s)
-- 
2.24.1