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

[Ganesh Ajjanagadde]

On Fri, Nov 20, 2015 at 9:48 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Fri, Nov 20, 2015 at 9:40 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> wrote:
>>
>> 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.
>
>
> Exactly that.
>
> Imagine I write a compiler and because I don't understand much of anything
> about compilers, whenever my internal state barfs, I just print "OOPS!" (and
> then it crashes). How helpful indeed! Can you imagine the stackoverflow
> comments this would invoke, assuming anyone is crazy enough to actually use
> it? To the average joe user, the above message is the equivalent of "OOPS!".
> It doesn't provide them any help on what went wrong, it doesn't tell them
> how to fix it, and since the message is duplicated all over the place, even
> an expert user (or developer) wouldn't be able to trace the specific
> invocation point through the code without re-running it himself in a debug
> build where each message is modified to be unique. Ergo: the message is
> meaningless.

Your analogy and "proof" are not strictly correct. It is not the same
as "OOPS": av_log messages do contain the context, so I can tell that
it arose from mp3dec or rmdec. It is thus not meaningless.

>
> Ronald