[FFmpeg-devel] [PATCH] svq1enc: fix out of bounds reads

[Michael Niedermayer]

On Tue, Jan 26, 2016 at 01:04:05AM +0100, Andreas Cadhalpun wrote:
> On 22.01.2016 00:57, Michael Niedermayer wrote:
> > On Thu, Jan 21, 2016 at 11:04:14PM +0100, Andreas Cadhalpun wrote:
> >> level can be up to 5, but there are only four codebooks.
> >>
> >> Fixes ubsan runtime error: index 5 out of bounds for type 'int8_t
> >> [4][96]'
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >> ---
> >>  libavcodec/svq1enc.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/libavcodec/svq1enc.c b/libavcodec/svq1enc.c
> >> index 1e1745e..7ff72b4 100644
> >> --- a/libavcodec/svq1enc.c
> >> +++ b/libavcodec/svq1enc.c
> >> @@ -104,7 +104,7 @@ static int encode_block(SVQ1EncContext *s, uint8_t *src, uint8_t *ref,
> >>      best_score = 0;
> >>      // FIXME: Optimize, this does not need to be done multiple times.
> >>      if (intra) {
> >> -        codebook_sum   = svq1_intra_codebook_sum[level];
> >> +        codebook_sum   = level < 4 ? svq1_intra_codebook_sum[level] : NULL;
> >>          codebook       = ff_svq1_intra_codebooks[level];
> >>          mean_vlc       = ff_svq1_intra_mean_vlc;
> >>          multistage_vlc = ff_svq1_intra_multistage_vlc[level];
> >> @@ -117,7 +117,7 @@ static int encode_block(SVQ1EncContext *s, uint8_t *src, uint8_t *ref,
> >>              }
> >>          }
> >>      } else {
> >> -        codebook_sum   = svq1_inter_codebook_sum[level];
> >> +        codebook_sum   = level < 4 ? svq1_inter_codebook_sum[level] : NULL;
> >>          codebook       = ff_svq1_inter_codebooks[level];
> >>          mean_vlc       = ff_svq1_inter_mean_vlc + 256;
> >>          multistage_vlc = ff_svq1_inter_multistage_vlc[level];
> > 
> >> @@ -143,7 +143,7 @@ static int encode_block(SVQ1EncContext *s, uint8_t *src, uint8_t *ref,
> >>              const int8_t *vector;
> >>  
> >>              for (i = 0; i < 16; i++) {
> >> -                int sum = codebook_sum[stage * 16 + i];
> >> +                int sum = codebook_sum ? codebook_sum[stage * 16 + i] : 0;
> >>                  int sqr, diff, score;
> > 
> > this is uneeded, it cannot be NULL
> 
> Indeed. That explains how FATE could pass...
> 
> > the other 2 checks should be commented to say something about the
> >> = 4 case being unused
> 
> Attached patch has comments there.
> 
> Best regards,
> Andreas
> 

>  svq1enc.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 303a9f08a561047395172ac4f7abbc63a78c9021  0001-svq1enc-fix-out-of-bounds-reads.patch
> From 5168bee94d1e7e09ebfcfe2bdab94430d4366cb2 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> Date: Thu, 21 Jan 2016 22:36:36 +0100
> Subject: [PATCH] svq1enc: fix out of bounds reads
> 
> level can be 5, but there are only four codebooks.
> 
> Fixes ubsan runtime error: index 5 out of bounds for type 'int8_t
> [4][96]'
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>

should be ok

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160126/46c3457f/attachment.sig>