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

[Ganesh Ajjanagadde]

On Wed, Nov 18, 2015 at 8:26 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> 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.

Another reason related to the randomness of the process killed for
OOM: ffmpeg/some other client may not be the process killed when OOM
occurs, and the malloc's here fail. Thus, seek back, something a user
does care about, fails, but without the error message, there is no
information whatsoever that such a thing failed and why. We should
make a "best effort" in such cases, especially since the alloc may be
non-trivial in size and has sufficient client impact.

Anyway, I by no means claim above patch is great, but I strongly
disagree with wm4 here.

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