[09/10] aic: Validate values read from the bitstream

Message ID 1378932481-98398-9-git-send-email-martin@martin.st
State Committed
Commit 0f678c0214dccb355ed8955077a2bea46984fbc8
Headers show

Commit Message

Martin Storsjö Sept. 11, 2013, 8:48 p.m.
Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC: libav-stable@libav.org
---
It appears that we don't have any fate tests for this - can
someone with samples please verify that this doesn't break
any common ones?
---
 libavcodec/aic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Kostya Shishkov Sept. 12, 2013, 4:37 a.m. | #1
On Wed, Sep 11, 2013 at 11:48:00PM +0300, Martin Storsjö wrote:
> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC: libav-stable@libav.org
> ---
> It appears that we don't have any fate tests for this - can
> someone with samples please verify that this doesn't break
> any common ones?

I have two or three samples, I'll test

> ---
>  libavcodec/aic.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/aic.c b/libavcodec/aic.c
> index e46c003..f295249 100644
> --- a/libavcodec/aic.c
> +++ b/libavcodec/aic.c
> @@ -215,12 +215,14 @@ static int aic_decode_coeffs(GetBitContext *gb, int16_t *dst,
>              idx = -1;
>              do {
>                  GET_CODE(val, skip_type, skip_bits);
> +                if (val < 0)
> +                    return AVERROR_INVALIDDATA;
>                  idx += val + 1;
>                  if (idx >= num_coeffs)
>                      break;
>                  GET_CODE(val, coeff_type, coeff_bits);
>                  val++;
> -                if (val >= 0x10000)
> +                if (val >= 0x10000 || val < 0)
>                      return AVERROR_INVALIDDATA;
>                  dst[scan[idx]] = val;
>              } while (idx < num_coeffs - 1);
> @@ -230,7 +232,7 @@ static int aic_decode_coeffs(GetBitContext *gb, int16_t *dst,
>          for (mb = 0; mb < slice_width; mb++) {
>              for (idx = 0; idx < num_coeffs; idx++) {
>                  GET_CODE(val, coeff_type, coeff_bits);
> -                if (val >= 0x10000)
> +                if (val >= 0x10000 || val < 0)
>                      return AVERROR_INVALIDDATA;
>                  dst[scan[idx]] = val;
>              }
> --

LGTM
Kostya Shishkov Sept. 12, 2013, 5:15 a.m. | #2
On Wed, Sep 11, 2013 at 11:48:00PM +0300, Martin Storsjö wrote:
> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC: libav-stable@libav.org
> ---
> It appears that we don't have any fate tests for this - can
> someone with samples please verify that this doesn't break
> any common ones?

I've tested all 4 samples I have (two progressive and two interlaced), all
work fine.

Patch

diff --git a/libavcodec/aic.c b/libavcodec/aic.c
index e46c003..f295249 100644
--- a/libavcodec/aic.c
+++ b/libavcodec/aic.c
@@ -215,12 +215,14 @@  static int aic_decode_coeffs(GetBitContext *gb, int16_t *dst,
             idx = -1;
             do {
                 GET_CODE(val, skip_type, skip_bits);
+                if (val < 0)
+                    return AVERROR_INVALIDDATA;
                 idx += val + 1;
                 if (idx >= num_coeffs)
                     break;
                 GET_CODE(val, coeff_type, coeff_bits);
                 val++;
-                if (val >= 0x10000)
+                if (val >= 0x10000 || val < 0)
                     return AVERROR_INVALIDDATA;
                 dst[scan[idx]] = val;
             } while (idx < num_coeffs - 1);
@@ -230,7 +232,7 @@  static int aic_decode_coeffs(GetBitContext *gb, int16_t *dst,
         for (mb = 0; mb < slice_width; mb++) {
             for (idx = 0; idx < num_coeffs; idx++) {
                 GET_CODE(val, coeff_type, coeff_bits);
-                if (val >= 0x10000)
+                if (val >= 0x10000 || val < 0)
                     return AVERROR_INVALIDDATA;
                 dst[scan[idx]] = val;
             }