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.cadhal...@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.cadhal...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
