[FFmpeg-devel] [PATCHv2] avformat/mp3dec, rmdec: check return value of ffio_ensure_seekback

[Ganesh Ajjanagadde]

On Wed, Nov 18, 2015 at 7:31 AM, wm4 <nfxjfg at gmail.com> wrote:
> On Tue, 17 Nov 2015 17:39:31 -0500
> Ganesh Ajjanagadde <gajjanagadde at gmail.com> wrote:
>
>> ffio_ensure_seekback can fail due to e.g ENOMEM. This return value is
>> checked here and a diagnostic is logged.
>>
>> All usage of ffio_ensure_seekback in the codebase now has the return value checked.
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> ---
>>  libavformat/mp3dec.c | 6 ++++--
>>  libavformat/rmdec.c  | 3 ++-
>>  2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
>> index 32ca00c..a14bccd 100644
>> --- a/libavformat/mp3dec.c
>> +++ b/libavformat/mp3dec.c
>> @@ -380,11 +380,13 @@ static int mp3_read_header(AVFormatContext *s)
>>          uint32_t header, header2;
>>          int frame_size;
>>          if (!(i&1023))
>> -            ffio_ensure_seekback(s->pb, i + 1024 + 4);
>> +            if ((ret = ffio_ensure_seekback(s->pb, i + 1024 + 4)) < 0)
>> +                av_log(s, AV_LOG_WARNING, "ffio_ensure_seekback(): %s\n", av_err2str(ret));
>>          frame_size = check(s->pb, off + i, &header);
>>          if (frame_size > 0) {
>>              avio_seek(s->pb, off, SEEK_SET);
>> -            ffio_ensure_seekback(s->pb, i + 1024 + frame_size + 4);
>> +            if ((ret = ffio_ensure_seekback(s->pb, i + 1024 + frame_size + 4)) < 0)
>> +                av_log(s, AV_LOG_WARNING, "ffio_ensure_seekback(): %s\n", av_err2str(ret));
>>              if (check(s->pb, off + i + frame_size, &header2) >= 0 &&
>>                  (header & SAME_HEADER_MASK) == (header2 & SAME_HEADER_MASK))
>>              {
>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>> index 4ec78ef..1cf0548 100644
>> --- a/libavformat/rmdec.c
>> +++ b/libavformat/rmdec.c
>> @@ -576,7 +576,8 @@ static int rm_read_header(AVFormatContext *s)
>>              size = avio_rb32(pb);
>>              codec_pos = avio_tell(pb);
>>
>> -            ffio_ensure_seekback(pb, 4);
>> +            if ((ret = ffio_ensure_seekback(pb, 4)) < 0)
>> +                av_log(s, AV_LOG_WARNING, "ffio_ensure_seekback(): %s\n", av_err2str(ret));
>>              v = avio_rb32(pb);
>>              if (v == MKBETAG('M', 'L', 'T', 'I')) {
>>                  int number_of_streams = avio_rb16(pb);
>
> So why do you not just add the message to the function itself?

Because a client may not like the generic message, see e.g the message
printed in avformat/apngdec. Handling of the error may vary from
context to context, again see the apngdec example where the client
does something else in addition to error printing.

>
> I also question the usefulness of the message. In such cases of OOM
> everything goes to hell anyway. No reason to bloat the code with error
> printing.

Taking that logic a bit further, why bother returning ENOMEM? If the
ENOMEM being returned by ensure_seekback is not checked, the
information is just silently discarded or garbled with further errors
down the road.

Anyway, I do not consider it a bloat: the buffer allocated can be big
(see in mp3dec > 1024 bytes). A user has a right to know when things
"go to hell". Envisioning what may happen, ffmpeg crashes due to OOM,
upon which a coredump takes place. That crash happens in a somewhat
random location, since OOM handling is a heuristic and determining
which process should be killed is an intractable problem. Thus at the
time when the core dump is written out, it is not guaranteed that the
context is the one where the allocation failed first. Then there are
issues of overcommit where malloc may return a non-NULL pointer but
OOM happens upon a dereference. Such issues are beyond the scope of
error handling, as nothing can be done in such cases anyway.

Warning the user when a reasonably large allocation failed in my view
is not bloat. It is giving them as much information as we can. Also
think of it from a debugging perspective: it can help us find places
where the alloc size was unreasonable.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel