diff mbox

[FFmpeg-devel,1/3] lavf/matroskadec: don't treat I/O errors as EOF

Message ID 20181128071959.30447-1-rodger.combs@gmail.com
State Withdrawn, archived
Headers show

Commit Message

Rodger Combs Nov. 28, 2018, 7:19 a.m. UTC 
pb->eof_reached is set on error, so we need to check pb->error,
even after checking pb->eof_reached or avio_feof(pb), or else we
can end up returning AVERROR_EOF instead of the actual error code.
---
 libavformat/matroskadec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Marton Balint Nov. 28, 2018, 7:19 p.m. UTC | #1 
On Wed, 28 Nov 2018, Rodger Combs wrote:

> pb->eof_reached is set on error, so we need to check pb->error,
> even after checking pb->eof_reached or avio_feof(pb), or else we
> can end up returning AVERROR_EOF instead of the actual error code.

Why eof_reached is set in the first place on error? Why does avio_feof() 
return nonzero if we are not at the end of file? That is strictly against 
its documentation.

Seems like a fine mess, let's think about how we will resolve this in a 
generic way.

Thanks,
Marton
Rodger Combs Nov. 28, 2018, 7:40 p.m. UTC | #2 
> On Nov 28, 2018, at 13:19, Marton Balint <cus@passwd.hu> wrote:
> 
> 
> 
> On Wed, 28 Nov 2018, Rodger Combs wrote:
> 
>> pb->eof_reached is set on error, so we need to check pb->error,
>> even after checking pb->eof_reached or avio_feof(pb), or else we
>> can end up returning AVERROR_EOF instead of the actual error code.
> 
> Why eof_reached is set in the first place on error? Why does avio_feof() return nonzero if we are not at the end of file? That is strictly against its documentation.

Looks like it's been like that since at least 2002. It's probably because a lot of code that loops on e.g. avio_r8 checks for EOF, but doesn't explicitly check for error, so if errors didn't set the EOF flag, they'd just loop forever. Either way, that code needs to be updated to return errors properly, so I don't think removing the EOF-on-error behavior is helpful (it'd just make existing problem cases worse).

> 
> Seems like a fine mess, let's think about how we will resolve this in a generic way.

I don't think this can really be resolved generically, since all the places that currently just return AVERROR_EOF in this case will need updating to read the error from AVIOContext regardless.

> 
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index d3c9c33720..2774ccabb2 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -789,7 +789,7 @@  static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
     }
 
     matroska->done = 1;
-    return AVERROR_EOF;
+    return pb->error ? pb->error : AVERROR_EOF;
 }
 
 /*
@@ -836,7 +836,7 @@  static int ebml_read_num(MatroskaDemuxContext *matroska, AVIOContext *pb,
                    pos, pos);
             return pb->error ? pb->error : AVERROR(EIO);
         }
-        return AVERROR_EOF;
+        return pb->error ? pb->error : AVERROR_EOF;
     }
 
     /* get the length of the EBML number */