[FFmpeg-devel] [PATCH] pnmdec: make sure v is capped by maxval

[Andreas Cadhalpun]

On 09.11.2016 21:55, Michael Niedermayer wrote:
> On Wed, Nov 09, 2016 at 09:05:17PM +0100, Andreas Cadhalpun wrote:
>> On 09.11.2016 11:10, Michael Niedermayer wrote:
>>> On Wed, Nov 09, 2016 at 01:11:29AM +0100, Andreas Cadhalpun wrote:
>>>> Otherwise put_bits can be called with a value that doesn't fit in the
>>>> sample_len, causing an assertion failure.
>>>> ---
>>>>  libavcodec/pnmdec.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/libavcodec/pnmdec.c b/libavcodec/pnmdec.c
>>>> index ca97cc3..0381ea6 100644
>>>> --- a/libavcodec/pnmdec.c
>>>> +++ b/libavcodec/pnmdec.c
>>>> @@ -145,6 +145,10 @@ static int pnm_decode_frame(AVCodecContext *avctx, void *data,
>>>>                          /* read a sequence of digits */
>>>>                          do {
>>>>                              v = 10*v + c;
>>>> +                             if (v > s->maxval) {
>>>> +                                av_log(avctx, AV_LOG_ERROR, "value %d larger than maxval %d\n", v, s->maxval);
>>>> +                                return AVERROR_INVALIDDATA;
>>>> +                            }
>>>
>>> indention is a bit noisy
>>
>> Fixed.
>>
>>> i think it can overflow if maxval is large,
>>
>> I've added an explicit check for v < 0, which should catch that.
>>
>>> it would be faster to check outside the loop
>>
>> However, such a check could pass if v overflowed so much that it's
>> in the valid range again, so I'd rather not do that.
>>
>> Updated patch is attached.
>>
>> Best regards,
>> Andreas
>>
> 
>>  pnmdec.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>> 9b84227c054610b73977e3acde32734a47e46c0c  0001-pnmdec-make-sure-v-is-capped-by-maxval.patch
>> From 7e9dcbde04ad95fc93ac2f0e91d734c8187c8d2b Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> Date: Wed, 9 Nov 2016 01:09:35 +0100
>> Subject: [PATCH] pnmdec: make sure v is capped by maxval
>>
>> Otherwise put_bits can be called with a value that doesn't fit in the
>> sample_len, causing an assertion failure.
>> ---
>>  libavcodec/pnmdec.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/libavcodec/pnmdec.c b/libavcodec/pnmdec.c
>> index ca97cc3..8961310 100644
>> --- a/libavcodec/pnmdec.c
>> +++ b/libavcodec/pnmdec.c
>> @@ -145,6 +145,10 @@ static int pnm_decode_frame(AVCodecContext *avctx, void *data,
>>                          /* read a sequence of digits */
>>                          do {
>>                              v = 10*v + c;
>> +                            if (v < 0 || v > s->maxval) {
> 
> v < 0 implies v is signed
> if 10*v overflows you have undefined behavior

Alright, let's use a more complex check. See attached patch.

Best regards,
Andreas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pnmdec-make-sure-v-is-capped-by-maxval.patch
Type: text/x-diff
Size: 1203 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161109/43da0422/attachment.patch>