[FFmpeg-devel,3/3] tiff: fix overflows when calling av_readuce

Submitted by Andreas Cadhalpun on Dec. 13, 2016, 11:57 p.m.

Details

Message ID ad3fbd69-c415-edc3-569d-76f8d5017f4c@googlemail.com
State New
Headers show

Commit Message

Andreas Cadhalpun Dec. 13, 2016, 11:57 p.m.
On 13.12.2016 01:32, Michael Niedermayer wrote:
> On Tue, Dec 13, 2016 at 12:50:19AM +0100, Andreas Cadhalpun wrote:
>> The arguments of av_reduce are signed, so the cast to uint64_t is misleading.
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavcodec/tiff.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
>> index 4721e94..12ef419 100644
>> --- a/libavcodec/tiff.c
>> +++ b/libavcodec/tiff.c
>> @@ -772,9 +772,16 @@ static void set_sar(TiffContext *s, unsigned tag, unsigned num, unsigned den)
>>      int offset = tag == TIFF_YRES ? 2 : 0;
>>      s->res[offset++] = num;
>>      s->res[offset]   = den;
>> -    if (s->res[0] && s->res[1] && s->res[2] && s->res[3])
>> +    if (s->res[0] && s->res[1] && s->res[2] && s->res[3]) {
>> +        uint64_t num = s->res[2] * (uint64_t)s->res[1];
>> +        uint64_t den = s->res[0] * (uint64_t)s->res[3];
>> +        if (num > INT64_MAX || den > INT64_MAX) {
>> +            num = num >> 1;
>> +            den = den >> 1;
>> +        }
> 
> this can make one of them 0, in fact i think even if they arent 0
> the sample_aspect_ratio can be  after reduce
> should they be checked after all that instead of before ?

I've added a check for !s->avctx->sample_aspect_ratio.den after av_reduce.
The check before is still necessary to prevent sample_aspect_ratio from
becoming negative.

Best regards,
Andreas

Comments

Michael Niedermayer Dec. 14, 2016, 12:34 p.m.
On Wed, Dec 14, 2016 at 12:57:12AM +0100, Andreas Cadhalpun wrote:
> On 13.12.2016 01:32, Michael Niedermayer wrote:
> > On Tue, Dec 13, 2016 at 12:50:19AM +0100, Andreas Cadhalpun wrote:
> >> The arguments of av_reduce are signed, so the cast to uint64_t is misleading.
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> >> ---
> >>  libavcodec/tiff.c | 11 +++++++++--
> >>  1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> >> index 4721e94..12ef419 100644
> >> --- a/libavcodec/tiff.c
> >> +++ b/libavcodec/tiff.c
> >> @@ -772,9 +772,16 @@ static void set_sar(TiffContext *s, unsigned tag, unsigned num, unsigned den)
> >>      int offset = tag == TIFF_YRES ? 2 : 0;
> >>      s->res[offset++] = num;
> >>      s->res[offset]   = den;
> >> -    if (s->res[0] && s->res[1] && s->res[2] && s->res[3])
> >> +    if (s->res[0] && s->res[1] && s->res[2] && s->res[3]) {
> >> +        uint64_t num = s->res[2] * (uint64_t)s->res[1];
> >> +        uint64_t den = s->res[0] * (uint64_t)s->res[3];
> >> +        if (num > INT64_MAX || den > INT64_MAX) {
> >> +            num = num >> 1;
> >> +            den = den >> 1;
> >> +        }
> > 
> > this can make one of them 0, in fact i think even if they arent 0
> > the sample_aspect_ratio can be  after reduce
> > should they be checked after all that instead of before ?
> 
> I've added a check for !s->avctx->sample_aspect_ratio.den after av_reduce.
> The check before is still necessary to prevent sample_aspect_ratio from
> becoming negative.
> 
> Best regards,
> Andreas
> 

>  tiff.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 96297593c98fcbce7395cb13627fae080f1e2cbc  0001-tiff-fix-overflows-when-calling-av_reduce.patch
> From 3cd8cb663d762bc15694e285ea48cdb8e9abfd4b Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> Date: Tue, 13 Dec 2016 00:43:21 +0100
> Subject: [PATCH] tiff: fix overflows when calling av_reduce
> 
> The arguments of av_reduce are signed, so the cast to uint64_t is misleading.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavcodec/tiff.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

LGTM

thx

[...]
Andreas Cadhalpun Dec. 15, 2016, 12:32 a.m.
On 14.12.2016 13:34, Michael Niedermayer wrote:
> On Wed, Dec 14, 2016 at 12:57:12AM +0100, Andreas Cadhalpun wrote:
>>  tiff.c |   13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>> 96297593c98fcbce7395cb13627fae080f1e2cbc  0001-tiff-fix-overflows-when-calling-av_reduce.patch
>> From 3cd8cb663d762bc15694e285ea48cdb8e9abfd4b Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> Date: Tue, 13 Dec 2016 00:43:21 +0100
>> Subject: [PATCH] tiff: fix overflows when calling av_reduce
>>
>> The arguments of av_reduce are signed, so the cast to uint64_t is misleading.
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavcodec/tiff.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> LGTM

Pushed.

Best regards,
Andreas

Patch hide | download patch | download mbox

From 3cd8cb663d762bc15694e285ea48cdb8e9abfd4b Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Tue, 13 Dec 2016 00:43:21 +0100
Subject: [PATCH] tiff: fix overflows when calling av_reduce

The arguments of av_reduce are signed, so the cast to uint64_t is misleading.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavcodec/tiff.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index 4721e94..7ccda51 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -772,9 +772,18 @@  static void set_sar(TiffContext *s, unsigned tag, unsigned num, unsigned den)
     int offset = tag == TIFF_YRES ? 2 : 0;
     s->res[offset++] = num;
     s->res[offset]   = den;
-    if (s->res[0] && s->res[1] && s->res[2] && s->res[3])
+    if (s->res[0] && s->res[1] && s->res[2] && s->res[3]) {
+        uint64_t num = s->res[2] * (uint64_t)s->res[1];
+        uint64_t den = s->res[0] * (uint64_t)s->res[3];
+        if (num > INT64_MAX || den > INT64_MAX) {
+            num = num >> 1;
+            den = den >> 1;
+        }
         av_reduce(&s->avctx->sample_aspect_ratio.num, &s->avctx->sample_aspect_ratio.den,
-                  s->res[2] * (uint64_t)s->res[1], s->res[0] * (uint64_t)s->res[3], INT32_MAX);
+                  num, den, INT32_MAX);
+        if (!s->avctx->sample_aspect_ratio.den)
+            s->avctx->sample_aspect_ratio = (AVRational) {0, 1};
+    }
 }
 
 static int tiff_decode_tag(TiffContext *s, AVFrame *frame)
-- 
2.10.2