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

[Michael Niedermayer]

On Wed, Nov 09, 2016 at 10:46:03PM +0100, Andreas Cadhalpun wrote:
> 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
> 

>  pnmdec.c |    4 ++++
>  1 file changed, 4 insertions(+)
> a970cb981be02ea692d0bf2e68976077f14f2de3  0001-pnmdec-make-sure-v-is-capped-by-maxval.patch
> From f315a3cfe35377a2638dc2294200a288408dc784 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..0f6a895 100644
> --- a/libavcodec/pnmdec.c
> +++ b/libavcodec/pnmdec.c
> @@ -144,6 +144,10 @@ static int pnm_decode_frame(AVCodecContext *avctx, void *data,
>                      } else {
>                          /* read a sequence of digits */
>                          do {
> +                            if (v > (INT_MAX - c) / 10 || 10 * v + c > s->maxval) {
> +                                av_log(avctx, AV_LOG_ERROR, "value 10 * %d + %d larger than maxval %d\n", v, c, s->maxval);
> +                                return AVERROR_INVALIDDATA;
> +                            }

this test should nt be inside the loop
you can try to benchmark with START/STOP_TIMER to see what effect
this has (i didnt try but i expect it to be bad), this is the
innermost loop of the decoder

you can just unroll the loop by 5, thats the max number of iterations
i think, which avoids the overflow
it also should make the code faster

iam reading in man ppm and others:
"The maximum color value (Maxval), again in ASCII decimal.  Must be less than 65536"

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20161110/4749d60d/attachment.sig>