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 |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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 >
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
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.
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
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.
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
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
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
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.
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
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.
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
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
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,
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