[FFmpeg-devel] [PATCH 12/37] avformat/matroskadec: Improve read error/EOF checks II

[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.

> +        }
> +        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;
>  }
>  
>