[FFmpeg-devel] [PATCH 12/37] avformat/matroskadec: Improve read error/EOF checks II
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Sun Jun 23 19:01:00 EEST 2019
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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list