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

[Ronald S. Bultje]

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.

Ronald