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

[James Almer]

On 6/23/2019 8:42 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>
> ---
> I did not combine the two lines via FFMIN as FFMIN might call avio_skip
> twice.
> 
>  libavformat/matroskadec.c | 59 +++++++++++++++++++++++++++------------
>  1 file changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index b923a9edff..1db7471440 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,
> @@ -869,7 +871,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)
>  {
> @@ -880,12 +882,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)
>  {
> @@ -901,12 +903,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)
>  {
> @@ -919,24 +921,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);
> @@ -947,7 +950,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)
>  {
> @@ -961,11 +964,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;
> @@ -1253,14 +1256,34 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska,
>      case EBML_STOP:
>          return 1;
>      default:
> -        if (ffio_limit(pb, length) != length)
> +        if (ffio_limit(pb, length) != length) {
> +            // ffio_limit emits its own error message,
> +            // so we don't have to.
>              return AVERROR(EIO);
> -        return avio_skip(pb, length) < 0 ? AVERROR(EIO) : 0;
> +        }
> +        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");
> +            res = AVERROR(EIO);
> +        }
>      }
> -    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;
>  }

Applied, thanks.