[FFmpeg-devel] [PATCHv2] avformat/mp3dec, rmdec: check return value of ffio_ensure_seekback
Michael Niedermayer
michael at niedermayer.cc
Wed Jan 6 01:57:50 CET 2016
On Tue, Jan 05, 2016 at 01:57:09PM -0800, Ganesh Ajjanagadde wrote:
> On Tue, Jan 5, 2016 at 11:01 AM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Tue, 5 Jan 2016 08:32:02 -0800
> > Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> >
> >> On Tue, Jan 5, 2016 at 5:29 AM, wm4 <nfxjfg at googlemail.com> wrote:
> >> > On Mon, 4 Jan 2016 17:50:01 -0800
> >> > 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.
> >> >>
> >> >> Reviewed-by: wm4 <nfxjfg at googlemail.com>
> >> >> Reviewed-by: Ronald S. Bultje <rsbultje at gmail.com>
> >> >> Reviewed-by: Michael Niedermayer <michael at niedermayer.cc>
> >> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> >> >> ---
> >> >> libavformat/mp3dec.c | 13 +++++++++++--
> >> >> libavformat/rmdec.c | 3 ++-
> >> >> 2 files changed, 13 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> >> >> index c76b21e..57ebcc8 100644
> >> >> --- a/libavformat/mp3dec.c
> >> >> +++ b/libavformat/mp3dec.c
> >> >> @@ -372,11 +372,19 @@ 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,
> >> >> + "initial junk detection and skipping impossible due to: %s\n", av_err2str(ret));
> >> >> + goto skip_fail;
> >> >> + }
> >> >> 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,
> >> >> + "initial junk detection and skipping impossible due to: %s\n", av_err2str(ret));
> >> >> + goto skip_fail;
> >> >> + }
> >> >> if (check(s->pb, off + i + frame_size, &header2) >= 0 &&
> >> >> (header & SAME_HEADER_MASK) == (header2 & SAME_HEADER_MASK))
> >> >> {
> >> >> @@ -387,6 +395,7 @@ static int mp3_read_header(AVFormatContext *s)
> >> >> }
> >> >> avio_seek(s->pb, off, SEEK_SET);
> >> >> }
> >> >> +skip_fail:
> >> >>
> >> >> // the seek index is relative to the end of the xing vbr headers
> >> >> for (i = 0; i < st->nb_index_entries; i++)
> >> >> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> >> >> index 4e46a3d..470e227 100644
> >> >> --- a/libavformat/rmdec.c
> >> >> +++ b/libavformat/rmdec.c
> >> >> @@ -618,7 +618,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, "seeking back impossible due to: %s\n", av_err2str(ret));
> >> >> v = avio_rb32(pb);
> >> >> if (v == MKBETAG('M', 'L', 'T', 'I')) {
> >> >> ret = rm_read_multi(s, s->pb, st, mime);
> >> >
> >> > I maintain that this should not be done, because it makes the code
> >> > verbose for no reason.
> >>
> >> Michael has pointed out that when seekback fails, one should really
> >> break out of the junk detection loop in mp3dec. Your idea fails to
> >> achieve it.
> >
> > Why? It's not really harmful. You could even argue that this will make
> > the common case (skipping jumk and finding a valid frame in a seekable
> > file) work in low memory situations, while your patch breaks it.
ffio_ensure_seekback() never fails for seekable files
it checks s->seekable and return 0 if true before attempting to
allocate anything
>
> I assumed Michael had a good reason for it, but from what you say here
> and my examination just now, I would at a first glance agree with you.
> Ronald had concerns about repetition of messages, so if he (and
> Michael) are fine with logging within ffio_ensure_seekback, then I
> will log it there.
as the code is written currently, if ffio_ensure_seekback() fails
and that being ignored, the url_seek() to return to the begin will
fail too quite likely while forward seeks will succeed.
this would result in random amounts of data to be lost at the begin
of the file
it may print errors or warnings if such are added but nothing (like
the applications return code) would indicate a problem, the user though
could end with a output file that is missing the first few seconds of
audio.
if we break out on a ffio_ensure_seekback() failure then problems
might still occur on files that have junk at te begin but files that
do not have junk (which is what a valid file should look like)
should work fine
all above is assuming iam not missing something ...
> If that is not fine, then the patch is dropped.
i think ignoring the error and printing a warning is still better
than leaving things as they are
> Thanks.
>
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are best at talking, realize last or never when they are wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160106/b763cb5a/attachment.sig>
More information about the ffmpeg-devel
mailing list