[FFmpeg-devel] lavf/mov: atom box parsing return eof cause play fail

Submitted by tiejun.peng on Dec. 7, 2017, 9:47 a.m.

Details

Message ID 1512640066-1401-1-git-send-email-tiejun.peng@foxmail.com
State New
Headers show

Commit Message

tiejun.peng Dec. 7, 2017, 9:47 a.m.
fix eof lead to play fail.

Signed-off-by: tiejun.peng <tiejun.peng@foxmail.com>
---
 libavformat/mov.c | 47 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)

Comments

Michael Niedermayer Dec. 7, 2017, 8:42 p.m.
On Thu, Dec 07, 2017 at 05:47:46PM +0800, tiejun.peng wrote:
> fix eof lead to play fail.
> 
> Signed-off-by: tiejun.peng <tiejun.peng@foxmail.com>
> ---
>  libavformat/mov.c | 47 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 11 deletions(-)

please split the addition of warning messages from the change to
EOF behavior

did you check that every EOF return case is safe to continue as if
no error occured ?
That change has quite wide effects possibly unless i misunderstand.


[...]
tiejun.peng Dec. 8, 2017, 4:06 p.m.
yes,  i have checked this case and i have done a lot of tests  with .mp4 file and fate

------------------ Original ------------------
From:  "Michael Niedermayer";<michael@niedermayer.cc>;

Send time: Friday, Dec 8, 2017 4:42 AM
To: "FFmpeg development discussions and patches"<ffmpeg-devel@ffmpeg.org>; 
Subject:  Re: [FFmpeg-devel] [PATCH] lavf/mov: atom box parsing return eofcause play fail

On Thu, Dec 07, 2017 at 05:47:46PM +0800, tiejun.peng wrote: > fix eof lead to play fail. >  > Signed-off-by: tiejun.peng <tiejun.peng at foxmail.com> > --- >  libavformat/mov.c | 47 ++++++++++++++++++++++++++++++++++++----------- >  1 file changed, 36 insertions(+), 11 deletions(-)  please split the addition of warning messages from the change to EOF behavior did you check that every EOF return case is safe to continue as if no error occured ? That change has quite wide effects possibly unless i misunderstand. [...] --  Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Michael Niedermayer Dec. 8, 2017, 7:36 p.m.
On Sat, Dec 09, 2017 at 12:06:47AM +0800, Tiejun.Peng wrote:
> yes,  i have checked this case and i have done a lot of tests  with .mp4 file and fate

Please correct me if iam wrong
there are many different atoms/boxes and many functions parsing
them
If any of these return EOF, it has previously been consideered an
error now it can be handled as not an error.

Noone has reviewed most of this code to be safe after the change.
It doesnt crash in fate or with some valid mp4 files

When a parsing function hits EOF it may return EOF.
This can occur in the middle of the function, initializing some but
not all of what it does normally.
Previously this would stop the demuxer and trigger cleanup, after
the change the code continues and may behave badly when it uses half
initialized structures



> 
> ------------------ Original ------------------
> From:  "Michael Niedermayer";<michael@niedermayer.cc>;
> Send time: Friday, Dec 8, 2017 4:42 AM
> To: "FFmpeg development discussions and patches"<ffmpeg-devel@ffmpeg.org>; 
> Subject:  Re: [FFmpeg-devel] [PATCH] lavf/mov: atom box parsing return eofcause play fail
> 
> On Thu, Dec 07, 2017 at 05:47:46PM +0800, tiejun.peng wrote: > fix eof lead to play fail. >  > Signed-off-by: tiejun.peng <tiejun.peng at foxmail.com> > --- >  libavformat/mov.c | 47 ++++++++++++++++++++++++++++++++++++----------- >  1 file changed, 36 insertions(+), 11 deletions(-)  please split the addition of warning messages from the change to EOF behavior did you check that every EOF return case is safe to continue as if no error occured ? That change has quite wide effects possibly unless i misunderstand. [...] --  Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
tiejun.peng Dec. 11, 2017, 12:37 p.m.
yes, it  is.
maybe we should add every stream's status and the whole file status can't  be fail only by  one of streams.
but the modification will be too much and the effect  is extensive.



------------------ Original ------------------
From:  "Michael Niedermayer";<michael@niedermayer.cc>;

Send time: Saturday, Dec 9, 2017 3:36 AM
To: "FFmpeg development discussions and patches"<ffmpeg-devel@ffmpeg.org>; 

Subject:  Re: [FFmpeg-devel] [PATCH] lavf/mov: atom box parsing returneofcause play fail

On Sat, Dec 09, 2017 at 12:06:47AM +0800, Tiejun.Peng wrote: > yes,  i have checked this case and i have done a lot of tests  with .mp4 file and fate  Please correct me if iam wrong there are many different atoms/boxes and many functions parsing them If any of these return EOF, it has previously been consideered an error now it can be handled as not an error. Noone has reviewed most of this code to be safe after the change. It doesnt crash in fate or with some valid mp4 files When a parsing function hits EOF it may return EOF. This can occur in the middle of the function, initializing some but not all of what it does normally. Previously this would stop the demuxer and trigger cleanup, after the change the code continues and may behave badly when it uses half initialized structures
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer Dec. 12, 2017, 11:49 p.m.
On Mon, Dec 11, 2017 at 08:37:43PM +0800, Tiejun.Peng wrote:
> yes, it  is.
> maybe we should add every stream's status and the whole file status can't  be fail only by  one of streams.
> but the modification will be too much and the effect  is extensive.

iam not sure i understand what you suggest but
this issue can be avoided by avoiding the overlap between the error
codes.
simply adding a otherwise not used error code for signaling that
data is truncated but everything is fine and code can continue
should work.
we can also use EOF as in your patch if people feel the advantages
outweight the risk. I just dont want this to be done as undocumented
sideeffect.
If EOF return implies everything must have been fully consistently
initialized then this must be documented

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/mov.c b/libavformat/mov.c
index c901859..6c3567f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1991,8 +1991,10 @@  static int mov_read_stco(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     sc->chunk_count = i;
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STCO atom\n");
         return AVERROR_EOF;
+    }
 
     return 0;
 }
@@ -2522,8 +2524,10 @@  int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext *pb, int entries)
         sc->stsd_count++;
     }
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSD atom\n");
         return AVERROR_EOF;
+    }
 
     return 0;
 }
@@ -2624,8 +2628,10 @@  static int mov_read_stsc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     sc->stsc_count = i;
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSC atom\n");
         return AVERROR_EOF;
+    }
 
     return 0;
 }
@@ -2676,8 +2682,10 @@  static int mov_read_stps(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     sc->stps_count = i;
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STPS atom\n");
         return AVERROR_EOF;
+    }
 
     return 0;
 }
@@ -2723,8 +2731,10 @@  static int mov_read_stss(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     sc->keyframe_count = i;
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSS atom\n");
         return AVERROR_EOF;
+    }
 
     return 0;
 }
@@ -2808,8 +2818,10 @@  static int mov_read_stsz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     av_free(buf);
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSZ atom\n");
         return AVERROR_EOF;
+    }
 
     return 0;
 }
@@ -2870,8 +2882,10 @@  static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     sc->duration_for_fps  += duration;
     sc->nb_frames_for_fps += total_sample_count;
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STTS atom\n");
         return AVERROR_EOF;
+    }
 
     st->nb_frames= total_sample_count;
     if (duration)
@@ -2948,8 +2962,10 @@  static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     sc->ctts_count = ctts_count;
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted CTTS atom\n");
         return AVERROR_EOF;
+    }
 
     av_log(c->fc, AV_LOG_TRACE, "dts shift %d\n", sc->dts_shift);
 
@@ -2995,7 +3011,12 @@  static int mov_read_sbgp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     sc->rap_group_count = i;
 
-    return pb->eof_reached ? AVERROR_EOF : 0;
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted SBGP atom\n");
+        return AVERROR_EOF;
+    }
+
+    return 0;
 }
 
 /**
@@ -4720,8 +4741,10 @@  static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     fix_frag_index_entries(&c->frag_index, next_frag_index,
                            frag->track_id, entries);
 
-    if (pb->eof_reached)
+    if (pb->eof_reached) {
+        av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted TRUN atom\n");
         return AVERROR_EOF;
+    }
 
     frag->implicit_offset = offset;
 
@@ -6609,7 +6632,9 @@  static int mov_read_header(AVFormatContext *s)
     do {
     if (mov->moov_retry)
         avio_seek(pb, 0, SEEK_SET);
-    if ((err = mov_read_default(mov, pb, atom)) < 0) {
+    /* EOF don't mean the file to play fail*/
+    err = mov_read_default(mov, pb, atom);
+    if (err < 0 && err != AVERROR_EOF) {
         av_log(s, AV_LOG_ERROR, "error reading header\n");
         mov_read_close(s);
         return err;