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

[Ganesh Ajjanagadde]

On Fri, Nov 20, 2015 at 9:30 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi Ganesh,
>
> On Fri, Nov 20, 2015 at 8:42 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> wrote:
>>
>> 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.
>
>
> He pointed out that you added multiple instances of the exact same line in
> the code:
>
> av_log(s, AV_LOG_WARNING, "ffio_ensure_seekback(): %s\n", av_err2str(ret));
>
> which is both duplicate as well as meaningless.

What is "meaningless" about it? Duplication I agree with, but I don't
know mp3dec, so I don't know what needs to be printed. All I am sure
of is some diagnostic needs to be present, that may vary across client
code.

> There is nothing to disagree
> with here, his criticism is just and needs to be addressed.

My reply is in my view "just" as well. Merely saying upon OOM, "things
go to hell anyway" is a cheap justification for "I don't care what
happens".

> Stop being
> defensive about trivial things like that.

It is not trivial, since it affects what needs to be done about when
ffio_ensure_seekback fails more generally. Call me "defensive" or
whatever else you like, but the points I raised above have not been
replied to regarding the need for (possibly tailored) diagnostics.

> Patch review is all about finding
> issues, fixing them, and moving on.

Of course, and I am interested in improvements as well.

>
> Ronald