| Message ID | 20190516223018.30827-13-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 5/16/2019 7:29 PM, Andreas Rheinhardt wrote: > This commit fixes a number of bugs: > > 1. There was no check that no read error/EOF occured during > ebml_read_uint, ebml_read_sint and ebml_read_float. > 2. ebml_read_ascii and ebml_read_binary did sometimes not forward > error codes; instead they simply returned AVERROR(EIO). > 3. In particular, AVERROR_EOF hasn't been used and no dedicated error > message for it existed. This has been changed. > > In order to reduce code duplication, the new error code NEEDS_CHECKING > has been introduced which makes ebml_parse check the AVIOContext's > status for errors. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/matroskadec.c | 59 ++++++++++++++++++++++++++------------- > 1 file changed, 40 insertions(+), 19 deletions(-) > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index 431e742a2d..3f11f60878 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -69,6 +69,8 @@ > #include "qtpalette.h" > > #define EBML_UNKNOWN_LENGTH UINT64_MAX /* EBML unknown length, in uint64_t */ > +#define NEEDS_CHECKING 2 /* Indicates that some error checks > + * still need to be performed */ > > typedef enum { > EBML_NONE, > @@ -891,7 +893,7 @@ static int ebml_read_length(MatroskaDemuxContext *matroska, AVIOContext *pb, > > /* > * Read the next element as an unsigned int. > - * 0 is success, < 0 is failure. > + * Returns NEEDS_CHECKING. > */ > static int ebml_read_uint(AVIOContext *pb, int size, uint64_t *num) > { > @@ -902,12 +904,12 @@ static int ebml_read_uint(AVIOContext *pb, int size, uint64_t *num) > while (n++ < size) > *num = (*num << 8) | avio_r8(pb); > > - return 0; > + return NEEDS_CHECKING; > } > > /* > * Read the next element as a signed int. > - * 0 is success, < 0 is failure. > + * Returns NEEDS_CHECKING. > */ > static int ebml_read_sint(AVIOContext *pb, int size, int64_t *num) > { > @@ -923,12 +925,12 @@ static int ebml_read_sint(AVIOContext *pb, int size, int64_t *num) > *num = ((uint64_t)*num << 8) | avio_r8(pb); > } > > - return 0; > + return NEEDS_CHECKING; > } > > /* > * Read the next element as a float. > - * 0 is success, < 0 is failure. > + * Returns NEEDS_CHECKING or < 0 on obvious failure. > */ > static int ebml_read_float(AVIOContext *pb, int size, double *num) > { > @@ -941,24 +943,25 @@ static int ebml_read_float(AVIOContext *pb, int size, double *num) > else > return AVERROR_INVALIDDATA; > > - return 0; > + return NEEDS_CHECKING; > } > > /* > * Read the next element as an ASCII string. > - * 0 is success, < 0 is failure. > + * 0 is success, < 0 or NEEDS_CHECKING is failure. > */ > static int ebml_read_ascii(AVIOContext *pb, int size, char **str) > { > char *res; > + int ret; > > /* EBML strings are usually not 0-terminated, so we allocate one > * byte more, read the string and NULL-terminate it ourselves. */ > if (!(res = av_malloc(size + 1))) > return AVERROR(ENOMEM); > - if (avio_read(pb, (uint8_t *) res, size) != size) { > + if ((ret = avio_read(pb, (uint8_t *) res, size)) != size) { > av_free(res); > - return AVERROR(EIO); > + return ret < 0 ? ret : NEEDS_CHECKING; > } > (res)[size] = '\0'; > av_free(*str); > @@ -969,7 +972,7 @@ static int ebml_read_ascii(AVIOContext *pb, int size, char **str) > > /* > * Read the next element as binary data. > - * 0 is success, < 0 is failure. > + * 0 is success, < 0 or NEEDS_CHECKING is failure. > */ > static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin) > { > @@ -983,11 +986,11 @@ static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin) > bin->data = bin->buf->data; > bin->size = length; > bin->pos = avio_tell(pb); > - if (avio_read(pb, bin->data, length) != length) { > + if ((ret = avio_read(pb, bin->data, length)) != length) { > av_buffer_unref(&bin->buf); > bin->data = NULL; > bin->size = 0; > - return AVERROR(EIO); > + return ret < 0 ? ret : NEEDS_CHECKING; > } > > return 0; > @@ -1277,14 +1280,32 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska, > case EBML_STOP: > return 1; > default: > - if (ffio_limit(pb, length) != length) > - return AVERROR(EIO); > - return avio_skip(pb, length) < 0 ? AVERROR(EIO) : 0; > + if (ffio_limit(pb, length) != length) { > + // ffio_limit emits its own error message, > + // so we don't have to. > + return AVERROR_EOF; AVERROR_EOF is not an error per se, it's used to signal that there's nothing else to read, and to finish demuxing. EOF here will not be treated as an error by the generic libavformat code, hence hardcoding EIO. > + } > + res = avio_skip(pb, length); > + res = res < 0 ? res : 0; These two lines can be simplified into res = FFMIN(avio_skip(pb, length), 0); > + } > + if (res) { > + if (res == NEEDS_CHECKING) { > + if (pb->eof_reached) { avio_feof()? > + if (pb->error) > + res = pb->error; > + else > + res = AVERROR_EOF; > + } else > + res = 0; > + } > + > + if (res == AVERROR_INVALIDDATA) > + av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n"); > + else if (res == AVERROR(EIO)) > + av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n"); > + else if (res == AVERROR_EOF) > + av_log(matroska->ctx, AV_LOG_ERROR, "File ended prematurely\n"); The custom message is fine, but you should change the res value into AVERROR(EIO) afterwards. > } > - if (res == AVERROR_INVALIDDATA) > - av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n"); > - else if (res == AVERROR(EIO)) > - av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n"); > return res; > } > >
James Almer: > On 5/16/2019 7:29 PM, Andreas Rheinhardt wrote: >> This commit fixes a number of bugs: >> >> 1. There was no check that no read error/EOF occured during >> ebml_read_uint, ebml_read_sint and ebml_read_float. >> 2. ebml_read_ascii and ebml_read_binary did sometimes not forward >> error codes; instead they simply returned AVERROR(EIO). >> 3. In particular, AVERROR_EOF hasn't been used and no dedicated error >> message for it existed. This has been changed. >> >> In order to reduce code duplication, the new error code NEEDS_CHECKING >> has been introduced which makes ebml_parse check the AVIOContext's >> status for errors. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/matroskadec.c | 59 ++++++++++++++++++++++++++------------- >> 1 file changed, 40 insertions(+), 19 deletions(-) >> >> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >> index 431e742a2d..3f11f60878 100644 >> --- a/libavformat/matroskadec.c >> +++ b/libavformat/matroskadec.c >> @@ -69,6 +69,8 @@ >> #include "qtpalette.h" >> >> #define EBML_UNKNOWN_LENGTH UINT64_MAX /* EBML unknown length, in uint64_t */ >> +#define NEEDS_CHECKING 2 /* Indicates that some error checks >> + * still need to be performed */ >> >> typedef enum { >> EBML_NONE, >> @@ -891,7 +893,7 @@ static int ebml_read_length(MatroskaDemuxContext *matroska, AVIOContext *pb, >> >> /* >> * Read the next element as an unsigned int. >> - * 0 is success, < 0 is failure. >> + * Returns NEEDS_CHECKING. >> */ >> static int ebml_read_uint(AVIOContext *pb, int size, uint64_t *num) >> { >> @@ -902,12 +904,12 @@ static int ebml_read_uint(AVIOContext *pb, int size, uint64_t *num) >> while (n++ < size) >> *num = (*num << 8) | avio_r8(pb); >> >> - return 0; >> + return NEEDS_CHECKING; >> } >> >> /* >> * Read the next element as a signed int. >> - * 0 is success, < 0 is failure. >> + * Returns NEEDS_CHECKING. >> */ >> static int ebml_read_sint(AVIOContext *pb, int size, int64_t *num) >> { >> @@ -923,12 +925,12 @@ static int ebml_read_sint(AVIOContext *pb, int size, int64_t *num) >> *num = ((uint64_t)*num << 8) | avio_r8(pb); >> } >> >> - return 0; >> + return NEEDS_CHECKING; >> } >> >> /* >> * Read the next element as a float. >> - * 0 is success, < 0 is failure. >> + * Returns NEEDS_CHECKING or < 0 on obvious failure. >> */ >> static int ebml_read_float(AVIOContext *pb, int size, double *num) >> { >> @@ -941,24 +943,25 @@ static int ebml_read_float(AVIOContext *pb, int size, double *num) >> else >> return AVERROR_INVALIDDATA; >> >> - return 0; >> + return NEEDS_CHECKING; >> } >> >> /* >> * Read the next element as an ASCII string. >> - * 0 is success, < 0 is failure. >> + * 0 is success, < 0 or NEEDS_CHECKING is failure. >> */ >> static int ebml_read_ascii(AVIOContext *pb, int size, char **str) >> { >> char *res; >> + int ret; >> >> /* EBML strings are usually not 0-terminated, so we allocate one >> * byte more, read the string and NULL-terminate it ourselves. */ >> if (!(res = av_malloc(size + 1))) >> return AVERROR(ENOMEM); >> - if (avio_read(pb, (uint8_t *) res, size) != size) { >> + if ((ret = avio_read(pb, (uint8_t *) res, size)) != size) { >> av_free(res); >> - return AVERROR(EIO); >> + return ret < 0 ? ret : NEEDS_CHECKING; >> } >> (res)[size] = '\0'; >> av_free(*str); >> @@ -969,7 +972,7 @@ static int ebml_read_ascii(AVIOContext *pb, int size, char **str) >> >> /* >> * Read the next element as binary data. >> - * 0 is success, < 0 is failure. >> + * 0 is success, < 0 or NEEDS_CHECKING is failure. >> */ >> static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin) >> { >> @@ -983,11 +986,11 @@ static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin) >> bin->data = bin->buf->data; >> bin->size = length; >> bin->pos = avio_tell(pb); >> - if (avio_read(pb, bin->data, length) != length) { >> + if ((ret = avio_read(pb, bin->data, length)) != length) { >> av_buffer_unref(&bin->buf); >> bin->data = NULL; >> bin->size = 0; >> - return AVERROR(EIO); >> + return ret < 0 ? ret : NEEDS_CHECKING; >> } >> >> return 0; >> @@ -1277,14 +1280,32 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska, >> case EBML_STOP: >> return 1; >> default: >> - if (ffio_limit(pb, length) != length) >> - return AVERROR(EIO); >> - return avio_skip(pb, length) < 0 ? AVERROR(EIO) : 0; >> + if (ffio_limit(pb, length) != length) { >> + // ffio_limit emits its own error message, >> + // so we don't have to. >> + return AVERROR_EOF; > > AVERROR_EOF is not an error per se, it's used to signal that there's > nothing else to read, and to finish demuxing. > EOF here will not be treated as an error by the generic libavformat > code, hence hardcoding EIO. > Ok, will change.>> + } >> + res = avio_skip(pb, length); >> + res = res < 0 ? res : 0; > > These two lines can be simplified into res = FFMIN(avio_skip(pb, > length), 0); > Will change, too. >> + } >> + if (res) { >> + if (res == NEEDS_CHECKING) { >> + if (pb->eof_reached) { > > avio_feof()? > I explained my rationale in the introduction: "I am unsure how to check whether a read error or attempted reading beyond EOF should be checked: Via avio_feof(pb) or via pb->eof_reached? The former tries to read again (refill the buffer) when pb->eof_reached was already set; does this mean that if one wants to know whether a read was not successfull one should check pb->eof_reached, but when one is more interested into whether one can perform future reads, one should use avio_feof (after all, in case of livestreaming, new data might have arrived after the earlier failed read)? That's at least the interpretation that I had when I wrote the patchset." Or to put it another way: If I want to read k bytes for (say) an uint, but only less bytes are available at the time of the attempted reading (avio_r8 will emit 0 for the unavailable), then it is possible that avio_feof will not catch that, because by the time of the check more data could be available. In this case the wrong value for the uint would be read and treated as correct. This affects not only this patch, but also at least the preceding one. >> + if (pb->error) >> + res = pb->error; >> + else >> + res = AVERROR_EOF; >> + } else >> + res = 0; >> + } >> + >> + if (res == AVERROR_INVALIDDATA) >> + av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n"); >> + else if (res == AVERROR(EIO)) >> + av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n"); >> + else if (res == AVERROR_EOF) >> + av_log(matroska->ctx, AV_LOG_ERROR, "File ended prematurely\n"); > > The custom message is fine, but you should change the res value into > AVERROR(EIO) afterwards. > Ok, will do. >> } >> - if (res == AVERROR_INVALIDDATA) >> - av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n"); >> - else if (res == AVERROR(EIO)) >> - av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n"); >> return res; >> } >> >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 6/23/2019 1:01 PM, Andreas Rheinhardt wrote: > James Almer: >> On 5/16/2019 7:29 PM, Andreas Rheinhardt wrote: >>> This commit fixes a number of bugs: >>> >>> 1. There was no check that no read error/EOF occured during >>> ebml_read_uint, ebml_read_sint and ebml_read_float. >>> 2. ebml_read_ascii and ebml_read_binary did sometimes not forward >>> error codes; instead they simply returned AVERROR(EIO). >>> 3. In particular, AVERROR_EOF hasn't been used and no dedicated error >>> message for it existed. This has been changed. >>> >>> In order to reduce code duplication, the new error code NEEDS_CHECKING >>> has been introduced which makes ebml_parse check the AVIOContext's >>> status for errors. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavformat/matroskadec.c | 59 ++++++++++++++++++++++++++------------- >>> 1 file changed, 40 insertions(+), 19 deletions(-) >>> >>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >>> index 431e742a2d..3f11f60878 100644 >>> --- a/libavformat/matroskadec.c >>> +++ b/libavformat/matroskadec.c >>> @@ -69,6 +69,8 @@ >>> #include "qtpalette.h" >>> >>> #define EBML_UNKNOWN_LENGTH UINT64_MAX /* EBML unknown length, in uint64_t */ >>> +#define NEEDS_CHECKING 2 /* Indicates that some error checks >>> + * still need to be performed */ >>> >>> typedef enum { >>> EBML_NONE, >>> @@ -891,7 +893,7 @@ static int ebml_read_length(MatroskaDemuxContext *matroska, AVIOContext *pb, >>> >>> /* >>> * Read the next element as an unsigned int. >>> - * 0 is success, < 0 is failure. >>> + * Returns NEEDS_CHECKING. >>> */ >>> static int ebml_read_uint(AVIOContext *pb, int size, uint64_t *num) >>> { >>> @@ -902,12 +904,12 @@ static int ebml_read_uint(AVIOContext *pb, int size, uint64_t *num) >>> while (n++ < size) >>> *num = (*num << 8) | avio_r8(pb); >>> >>> - return 0; >>> + return NEEDS_CHECKING; >>> } >>> >>> /* >>> * Read the next element as a signed int. >>> - * 0 is success, < 0 is failure. >>> + * Returns NEEDS_CHECKING. >>> */ >>> static int ebml_read_sint(AVIOContext *pb, int size, int64_t *num) >>> { >>> @@ -923,12 +925,12 @@ static int ebml_read_sint(AVIOContext *pb, int size, int64_t *num) >>> *num = ((uint64_t)*num << 8) | avio_r8(pb); >>> } >>> >>> - return 0; >>> + return NEEDS_CHECKING; >>> } >>> >>> /* >>> * Read the next element as a float. >>> - * 0 is success, < 0 is failure. >>> + * Returns NEEDS_CHECKING or < 0 on obvious failure. >>> */ >>> static int ebml_read_float(AVIOContext *pb, int size, double *num) >>> { >>> @@ -941,24 +943,25 @@ static int ebml_read_float(AVIOContext *pb, int size, double *num) >>> else >>> return AVERROR_INVALIDDATA; >>> >>> - return 0; >>> + return NEEDS_CHECKING; >>> } >>> >>> /* >>> * Read the next element as an ASCII string. >>> - * 0 is success, < 0 is failure. >>> + * 0 is success, < 0 or NEEDS_CHECKING is failure. >>> */ >>> static int ebml_read_ascii(AVIOContext *pb, int size, char **str) >>> { >>> char *res; >>> + int ret; >>> >>> /* EBML strings are usually not 0-terminated, so we allocate one >>> * byte more, read the string and NULL-terminate it ourselves. */ >>> if (!(res = av_malloc(size + 1))) >>> return AVERROR(ENOMEM); >>> - if (avio_read(pb, (uint8_t *) res, size) != size) { >>> + if ((ret = avio_read(pb, (uint8_t *) res, size)) != size) { >>> av_free(res); >>> - return AVERROR(EIO); >>> + return ret < 0 ? ret : NEEDS_CHECKING; >>> } >>> (res)[size] = '\0'; >>> av_free(*str); >>> @@ -969,7 +972,7 @@ static int ebml_read_ascii(AVIOContext *pb, int size, char **str) >>> >>> /* >>> * Read the next element as binary data. >>> - * 0 is success, < 0 is failure. >>> + * 0 is success, < 0 or NEEDS_CHECKING is failure. >>> */ >>> static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin) >>> { >>> @@ -983,11 +986,11 @@ static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin) >>> bin->data = bin->buf->data; >>> bin->size = length; >>> bin->pos = avio_tell(pb); >>> - if (avio_read(pb, bin->data, length) != length) { >>> + if ((ret = avio_read(pb, bin->data, length)) != length) { >>> av_buffer_unref(&bin->buf); >>> bin->data = NULL; >>> bin->size = 0; >>> - return AVERROR(EIO); >>> + return ret < 0 ? ret : NEEDS_CHECKING; >>> } >>> >>> return 0; >>> @@ -1277,14 +1280,32 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska, >>> case EBML_STOP: >>> return 1; >>> default: >>> - if (ffio_limit(pb, length) != length) >>> - return AVERROR(EIO); >>> - return avio_skip(pb, length) < 0 ? AVERROR(EIO) : 0; >>> + if (ffio_limit(pb, length) != length) { >>> + // ffio_limit emits its own error message, >>> + // so we don't have to. >>> + return AVERROR_EOF; >> >> AVERROR_EOF is not an error per se, it's used to signal that there's >> nothing else to read, and to finish demuxing. >> EOF here will not be treated as an error by the generic libavformat >> code, hence hardcoding EIO. >> > Ok, will change.>> + } >>> + res = avio_skip(pb, length); >>> + res = res < 0 ? res : 0; >> >> These two lines can be simplified into res = FFMIN(avio_skip(pb, >> length), 0); >> > Will change, too. >>> + } >>> + if (res) { >>> + if (res == NEEDS_CHECKING) { >>> + if (pb->eof_reached) { >> >> avio_feof()? >> > I explained my rationale in the introduction: > "I am unsure how to check whether a read error or attempted reading > beyond EOF should be checked: Via avio_feof(pb) or via pb->eof_reached? > The former tries to read again (refill the buffer) when > pb->eof_reached was already set; does this mean that if one wants to > know whether a read was not successfull one should check > pb->eof_reached, but when one is more interested into whether one can > perform future reads, one should use avio_feof (after all, in case of > livestreaming, new data might have arrived after the earlier failed > read)? That's at least the interpretation that I had when I wrote the > patchset." > Or to put it another way: If I want to read k bytes for (say) an uint, > but only less bytes are available at the time of the attempted reading > (avio_r8 will emit 0 for the unavailable), then it is possible that > avio_feof will not catch that, because by the time of the check more > data could be available. In this case the wrong value for the uint > would be read and treated as correct. > This affects not only this patch, but also at least the preceding one. Ok, leave it as pb->eof_reached. I see it's also checked elsewhere in the demuxer, so it should be ok. > >>> + if (pb->error) >>> + res = pb->error; >>> + else >>> + res = AVERROR_EOF; >>> + } else >>> + res = 0; >>> + } >>> + >>> + if (res == AVERROR_INVALIDDATA) >>> + av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n"); >>> + else if (res == AVERROR(EIO)) >>> + av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n"); >>> + else if (res == AVERROR_EOF) >>> + av_log(matroska->ctx, AV_LOG_ERROR, "File ended prematurely\n"); >> >> The custom message is fine, but you should change the res value into >> AVERROR(EIO) afterwards. >> > Ok, will do. >>> } >>> - if (res == AVERROR_INVALIDDATA) >>> - av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n"); >>> - else if (res == AVERROR(EIO)) >>> - av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n"); >>> return res; >>> } >>> >>> >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 431e742a2d..3f11f60878 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -69,6 +69,8 @@ #include "qtpalette.h" #define EBML_UNKNOWN_LENGTH UINT64_MAX /* EBML unknown length, in uint64_t */ +#define NEEDS_CHECKING 2 /* Indicates that some error checks + * still need to be performed */ typedef enum { EBML_NONE, @@ -891,7 +893,7 @@ static int ebml_read_length(MatroskaDemuxContext *matroska, AVIOContext *pb, /* * Read the next element as an unsigned int. - * 0 is success, < 0 is failure. + * Returns NEEDS_CHECKING. */ static int ebml_read_uint(AVIOContext *pb, int size, uint64_t *num) { @@ -902,12 +904,12 @@ static int ebml_read_uint(AVIOContext *pb, int size, uint64_t *num) while (n++ < size) *num = (*num << 8) | avio_r8(pb); - return 0; + return NEEDS_CHECKING; } /* * Read the next element as a signed int. - * 0 is success, < 0 is failure. + * Returns NEEDS_CHECKING. */ static int ebml_read_sint(AVIOContext *pb, int size, int64_t *num) { @@ -923,12 +925,12 @@ static int ebml_read_sint(AVIOContext *pb, int size, int64_t *num) *num = ((uint64_t)*num << 8) | avio_r8(pb); } - return 0; + return NEEDS_CHECKING; } /* * Read the next element as a float. - * 0 is success, < 0 is failure. + * Returns NEEDS_CHECKING or < 0 on obvious failure. */ static int ebml_read_float(AVIOContext *pb, int size, double *num) { @@ -941,24 +943,25 @@ static int ebml_read_float(AVIOContext *pb, int size, double *num) else return AVERROR_INVALIDDATA; - return 0; + return NEEDS_CHECKING; } /* * Read the next element as an ASCII string. - * 0 is success, < 0 is failure. + * 0 is success, < 0 or NEEDS_CHECKING is failure. */ static int ebml_read_ascii(AVIOContext *pb, int size, char **str) { char *res; + int ret; /* EBML strings are usually not 0-terminated, so we allocate one * byte more, read the string and NULL-terminate it ourselves. */ if (!(res = av_malloc(size + 1))) return AVERROR(ENOMEM); - if (avio_read(pb, (uint8_t *) res, size) != size) { + if ((ret = avio_read(pb, (uint8_t *) res, size)) != size) { av_free(res); - return AVERROR(EIO); + return ret < 0 ? ret : NEEDS_CHECKING; } (res)[size] = '\0'; av_free(*str); @@ -969,7 +972,7 @@ static int ebml_read_ascii(AVIOContext *pb, int size, char **str) /* * Read the next element as binary data. - * 0 is success, < 0 is failure. + * 0 is success, < 0 or NEEDS_CHECKING is failure. */ static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin) { @@ -983,11 +986,11 @@ static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin) bin->data = bin->buf->data; bin->size = length; bin->pos = avio_tell(pb); - if (avio_read(pb, bin->data, length) != length) { + if ((ret = avio_read(pb, bin->data, length)) != length) { av_buffer_unref(&bin->buf); bin->data = NULL; bin->size = 0; - return AVERROR(EIO); + return ret < 0 ? ret : NEEDS_CHECKING; } return 0; @@ -1277,14 +1280,32 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska, case EBML_STOP: return 1; default: - if (ffio_limit(pb, length) != length) - return AVERROR(EIO); - return avio_skip(pb, length) < 0 ? AVERROR(EIO) : 0; + if (ffio_limit(pb, length) != length) { + // ffio_limit emits its own error message, + // so we don't have to. + return AVERROR_EOF; + } + res = avio_skip(pb, length); + res = res < 0 ? res : 0; + } + if (res) { + if (res == NEEDS_CHECKING) { + if (pb->eof_reached) { + if (pb->error) + res = pb->error; + else + res = AVERROR_EOF; + } else + res = 0; + } + + if (res == AVERROR_INVALIDDATA) + av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n"); + else if (res == AVERROR(EIO)) + av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n"); + else if (res == AVERROR_EOF) + av_log(matroska->ctx, AV_LOG_ERROR, "File ended prematurely\n"); } - if (res == AVERROR_INVALIDDATA) - av_log(matroska->ctx, AV_LOG_ERROR, "Invalid element\n"); - else if (res == AVERROR(EIO)) - av_log(matroska->ctx, AV_LOG_ERROR, "Read error\n"); return res; }
This commit fixes a number of bugs: 1. There was no check that no read error/EOF occured during ebml_read_uint, ebml_read_sint and ebml_read_float. 2. ebml_read_ascii and ebml_read_binary did sometimes not forward error codes; instead they simply returned AVERROR(EIO). 3. In particular, AVERROR_EOF hasn't been used and no dedicated error message for it existed. This has been changed. In order to reduce code duplication, the new error code NEEDS_CHECKING has been introduced which makes ebml_parse check the AVIOContext's status for errors. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/matroskadec.c | 59 ++++++++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 19 deletions(-)