| Message ID | 20200504182250.26141-10-andreas.rheinhardt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/10] libavformat/nutenc: Remove redundant function parameter | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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 [...]
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
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 --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];
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(-)