[FFmpeg-devel] [PATCH 12/37] avformat/matroskadec: Improve read error/EOF checks II
James Almer
jamrial at gmail.com
Sun Jun 23 19:29:41 EEST 2019
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 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.
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 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".
>>
>
> _______________________________________________
> 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