diff mbox series

[FFmpeg-devel,10/10] avformat/nutenc: Don't allocate array with zero entries

Message ID 20200504182250.26141-10-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,01/10] libavformat/nutenc: Remove redundant function parameter
Related show

Checks

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

Commit Message

Andreas Rheinhardt May 4, 2020, 6:22 p.m. UTC
Allocating an array with zero entries is both unnecessary as well as
potentially troublesome because the behaviour in this case is not really
well defined.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/nutenc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer May 8, 2020, 11:06 p.m. UTC | #1
On Mon, May 04, 2020 at 08:22:50PM +0200, Andreas Rheinhardt wrote:
> Allocating an array with zero entries is both unnecessary as well as
> potentially troublesome because the behaviour in this case is not really
> well defined.

What is not well defined ?

thx

[...]
Andreas Rheinhardt May 9, 2020, 4:52 a.m. UTC | #2
Michael Niedermayer:
> On Mon, May 04, 2020 at 08:22:50PM +0200, Andreas Rheinhardt wrote:
>> Allocating an array with zero entries is both unnecessary as well as
>> potentially troublesome because the behaviour in this case is not really
>> well defined.
> 
> What is not well defined ?
> 
The behaviour of our memory-allocation functions in case one requests to
allocate zero bytes is not well-defined (does it return NULL or not?).
E.g. although the documentation of av_mallocz_array() (which applies to
av_calloc, too) treats the size and nmemb parameters symmetrically, the
actual implementation differed until commit
b7d9507bb8c4d1b8bf99158d6859a5b2ecd73298 (up until then, a size of zero
would have resulted in NULL being returned). In the discussion [1]
around this patch there were devs favouring returning NULL upon any
request to allocate zero bytes. The NUT muxer is btw one of the two main
reasons for failing fate-tests in case one returned NULL when an
allocation of zero bytes is requested.

What happens on allocation of zero bytes is implementation-defined for
the analogous C functions; in our implementation the only function that
explicitly documents what happens when the size fed to it is zero is
av_realloc() which does not abide by its documentation (it does not free
anything when size is zero, but rather reallocates the buffer to a size
of one).

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260493.html
Michael Niedermayer May 9, 2020, 3 p.m. UTC | #3
On Sat, May 09, 2020 at 06:52:35AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Mon, May 04, 2020 at 08:22:50PM +0200, Andreas Rheinhardt wrote:
> >> Allocating an array with zero entries is both unnecessary as well as
> >> potentially troublesome because the behaviour in this case is not really
> >> well defined.
> > 
> > What is not well defined ?
> > 
> The behaviour of our memory-allocation functions in case one requests to
> allocate zero bytes is not well-defined (does it return NULL or not?).
> E.g. although the documentation of av_mallocz_array() (which applies to
> av_calloc, too) treats the size and nmemb parameters symmetrically, the
> actual implementation differed until commit
> b7d9507bb8c4d1b8bf99158d6859a5b2ecd73298 (up until then, a size of zero
> would have resulted in NULL being returned). In the discussion [1]
> around this patch there were devs favouring returning NULL upon any
> request to allocate zero bytes. The NUT muxer is btw one of the two main
> reasons for failing fate-tests in case one returned NULL when an
> allocation of zero bytes is requested.
> 
> What happens on allocation of zero bytes is implementation-defined for
> the analogous C functions; in our implementation the only function that
> explicitly documents what happens when the size fed to it is zero is

IMO The intended behavor of the allocation functions was to return NULL on
failure only.

That way a NULL test can be used for testing for failure without special
cases. Also it avoids special cases with NULL pointers with functions which
do not allow NULL even for size = 0.
I think at minimum a succeeding *malloc(x) should work with *memcpy(x)/*memset(x)/...
of the same API.
if the later are undefined with NULL so the first should not produce NULL on
success.

> av_realloc() which does not abide by its documentation (it does not free
> anything when size is zero, but rather reallocates the buffer to a size
> of one).

Thats an interresting bug

thx


[...]
diff mbox series

Patch

diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
index 6df7dfe210..5071278835 100644
--- a/libavformat/nutenc.c
+++ b/libavformat/nutenc.c
@@ -711,11 +711,15 @@  static int nut_write_header(AVFormatContext *s)
     }
 
     nut->stream   = av_calloc(s->nb_streams,  sizeof(*nut->stream ));
-    nut->chapter  = av_calloc(s->nb_chapters, sizeof(*nut->chapter));
     nut->time_base= av_calloc(s->nb_streams +
                               s->nb_chapters, sizeof(*nut->time_base));
-    if (!nut->stream || !nut->chapter || !nut->time_base)
+    if (!nut->stream || !nut->time_base)
         return AVERROR(ENOMEM);
+    if (s->nb_chapters) {
+        nut->chapter = av_calloc(s->nb_chapters, sizeof(*nut->chapter));
+        if (!nut->chapter)
+            return AVERROR(ENOMEM);
+    }
 
     for (i = 0; i < s->nb_streams; i++) {
         AVStream *st = s->streams[i];