[05/17] alac: Do bounds checking of lpc_order read from the bitstream
diff mbox

Message ID 1380406879-6174-5-git-send-email-martin@martin.st
State Committed
Commit 59480abce7e4238e22b3a4a904a9fe6abf4e4188
Headers show

Commit Message

Martin Storsjö Sept. 28, 2013, 10:21 p.m. 
In lpc_prediction(), we write up to array element 'lpc_order' in
an array allocated to hold 'max_samples_per_frame' elements.

Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC: libav-stable@libav.org
---
 libavcodec/alac.c |    3 +++
 1 file changed, 3 insertions(+)

Comments

Kostya Shishkov Sept. 29, 2013, 5:56 a.m. | #1 
On Sun, Sep 29, 2013 at 01:21:07AM +0300, Martin Storsjö wrote:
> In lpc_prediction(), we write up to array element 'lpc_order' in
> an array allocated to hold 'max_samples_per_frame' elements.
> 
> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC: libav-stable@libav.org
> ---
>  libavcodec/alac.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/alac.c b/libavcodec/alac.c
> index 41d1f77..6d1ace3 100644
> --- a/libavcodec/alac.c
> +++ b/libavcodec/alac.c
> @@ -314,6 +314,9 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index,
>              rice_history_mult[ch] = get_bits(&alac->gb, 3);
>              lpc_order[ch]         = get_bits(&alac->gb, 5);
>  
> +            if (lpc_order[ch] >= alac->max_samples_per_frame)
> +                return AVERROR_INVALIDDATA;
> +
>              /* read the predictor table */
>              for (i = lpc_order[ch] - 1; i >= 0; i--)
>                  lpc_coefs[ch][i] = get_sbits(&alac->gb, 16);
> -- 

looks a bit strange, I'd expect lpc_order > max_samples_per_frame
Martin Storsjö Sept. 29, 2013, 9:36 a.m. | #2 
On Sun, 29 Sep 2013, Kostya Shishkov wrote:

> On Sun, Sep 29, 2013 at 01:21:07AM +0300, Martin Storsjö wrote:
>> In lpc_prediction(), we write up to array element 'lpc_order' in
>> an array allocated to hold 'max_samples_per_frame' elements.
>>
>> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
>> CC: libav-stable@libav.org
>> ---
>>  libavcodec/alac.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/libavcodec/alac.c b/libavcodec/alac.c
>> index 41d1f77..6d1ace3 100644
>> --- a/libavcodec/alac.c
>> +++ b/libavcodec/alac.c
>> @@ -314,6 +314,9 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index,
>>              rice_history_mult[ch] = get_bits(&alac->gb, 3);
>>              lpc_order[ch]         = get_bits(&alac->gb, 5);
>>
>> +            if (lpc_order[ch] >= alac->max_samples_per_frame)
>> +                return AVERROR_INVALIDDATA;
>> +
>>              /* read the predictor table */
>>              for (i = lpc_order[ch] - 1; i >= 0; i--)
>>                  lpc_coefs[ch][i] = get_sbits(&alac->gb, 16);
>> --
>
> looks a bit strange, I'd expect lpc_order > max_samples_per_frame

In practice it isn't - in some random samples, lpc_order is 5-30 or so, 
while max_samples_per_frame is 4096.

The loop that writes out of bounds is this one, in lpc_prediction:

     for (i = 1; i <= lpc_order; i++)
         buffer_out[i] = sign_extend(buffer_out[i - 1] + error_buffer[i], bps);

OTOH, if lpc_order > max_samples_per_frame is to be expected, we could 
perhaps just add an FFMIN() to the loop?

// Martin
Kostya Shishkov Sept. 29, 2013, 11:03 a.m. | #3 
On Sun, Sep 29, 2013 at 12:36:03PM +0300, Martin Storsjö wrote:
> On Sun, 29 Sep 2013, Kostya Shishkov wrote:
> 
> >On Sun, Sep 29, 2013 at 01:21:07AM +0300, Martin Storsjö wrote:
> >>In lpc_prediction(), we write up to array element 'lpc_order' in
> >>an array allocated to hold 'max_samples_per_frame' elements.
> >>
> >>Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> >>CC: libav-stable@libav.org
> >>---
> >> libavcodec/alac.c |    3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >>diff --git a/libavcodec/alac.c b/libavcodec/alac.c
> >>index 41d1f77..6d1ace3 100644
> >>--- a/libavcodec/alac.c
> >>+++ b/libavcodec/alac.c
> >>@@ -314,6 +314,9 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index,
> >>             rice_history_mult[ch] = get_bits(&alac->gb, 3);
> >>             lpc_order[ch]         = get_bits(&alac->gb, 5);
> >>
> >>+            if (lpc_order[ch] >= alac->max_samples_per_frame)
> >>+                return AVERROR_INVALIDDATA;
> >>+
> >>             /* read the predictor table */
> >>             for (i = lpc_order[ch] - 1; i >= 0; i--)
> >>                 lpc_coefs[ch][i] = get_sbits(&alac->gb, 16);
> >>--
> >
> >looks a bit strange, I'd expect lpc_order > max_samples_per_frame
> 
> In practice it isn't - in some random samples, lpc_order is 5-30 or
> so, while max_samples_per_frame is 4096.
> 
> The loop that writes out of bounds is this one, in lpc_prediction:
> 
>     for (i = 1; i <= lpc_order; i++)
>         buffer_out[i] = sign_extend(buffer_out[i - 1] + error_buffer[i], bps);
> 
> OTOH, if lpc_order > max_samples_per_frame is to be expected, we
> could perhaps just add an FFMIN() to the loop?

No, the patch is fine then. There's no point of having filter larger than
max_samples (and having such small max_samples value either).

Patch
diff mbox 

diff --git a/libavcodec/alac.c b/libavcodec/alac.c
index 41d1f77..6d1ace3 100644
--- a/libavcodec/alac.c
+++ b/libavcodec/alac.c
@@ -314,6 +314,9 @@  static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index,
             rice_history_mult[ch] = get_bits(&alac->gb, 3);
             lpc_order[ch]         = get_bits(&alac->gb, 5);
 
+            if (lpc_order[ch] >= alac->max_samples_per_frame)
+                return AVERROR_INVALIDDATA;
+
             /* read the predictor table */
             for (i = lpc_order[ch] - 1; i >= 0; i--)
                 lpc_coefs[ch][i] = get_sbits(&alac->gb, 16);