[FFmpeg-devel,4/5] avcodec/fitsdec: Use lrint()

Submitted by Michael Niedermayer on Sept. 30, 2019, 4:30 p.m.

Details

Message ID 20190930163027.11686-4-michael@niedermayer.cc
State Accepted
Commit 37f31f4e509fe4ccc56a64edaa6fa3d95ee20466
Headers show

Commit Message

Michael Niedermayer Sept. 30, 2019, 4:30 p.m.
Fixes: -nan is outside the range of representable values of type 'unsigned short'
Fixes: 17769/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5678314672357376

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/fitsdec.c                    | 2 +-
 tests/ref/fate/fitsdec-bitpix-32        | 2 +-
 tests/ref/fate/fitsdec-bitpix-64        | 2 +-
 tests/ref/fate/fitsdec-blank_bitpix32   | 2 +-
 tests/ref/fate/fitsdec-ext_data_min_max | 2 +-
 tests/ref/fate/fitsdec-gray             | 2 +-
 tests/ref/lavf/gray16be.fits            | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

Comments

James Almer Oct. 1, 2019, 1:30 a.m.
On 9/30/2019 1:30 PM, Michael Niedermayer wrote:
> Fixes: -nan is outside the range of representable values of type 'unsigned short'

From lrint documentation:

"If x is a NaN or an infinity, or the rounded value is too large to be
stored in a long (long long in the case of the ll* functions), then a
domain error occurs, and the return value is unspecified."

So i don't know if using lrint is a good idea here.

> Fixes: 17769/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5678314672357376

Is the output of av_int2double/av_int2float or header.data_min NaN in
that testcase? Wouldn't it be better to check that instead, and abort?

> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/fitsdec.c                    | 2 +-
>  tests/ref/fate/fitsdec-bitpix-32        | 2 +-
>  tests/ref/fate/fitsdec-bitpix-64        | 2 +-
>  tests/ref/fate/fitsdec-blank_bitpix32   | 2 +-
>  tests/ref/fate/fitsdec-ext_data_min_max | 2 +-
>  tests/ref/fate/fitsdec-gray             | 2 +-
>  tests/ref/lavf/gray16be.fits            | 2 +-
>  7 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/fitsdec.c b/libavcodec/fitsdec.c
> index a20b8faf9e..2bb215943c 100644
> --- a/libavcodec/fitsdec.c
> +++ b/libavcodec/fitsdec.c
> @@ -279,7 +279,7 @@ static int fits_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
>              for (j = 0; j < avctx->width; j++) { \
>                  t = rd; \
>                  if (!header.blank_found || t != header.blank) { \
> -                    *dst++ = ((t - header.data_min) * ((1 << (sizeof(type) * 8)) - 1)) * scale; \
> +                    *dst++ = lrint(((t - header.data_min) * ((1 << (sizeof(type) * 8)) - 1)) * scale); \
>                  } else { \
>                      *dst++ = fitsctx->blank_val; \
>                  } \
> diff --git a/tests/ref/fate/fitsdec-bitpix-32 b/tests/ref/fate/fitsdec-bitpix-32
> index 9bce361555..b3a51401d4 100644
> --- a/tests/ref/fate/fitsdec-bitpix-32
> +++ b/tests/ref/fate/fitsdec-bitpix-32
> @@ -3,4 +3,4 @@
>  #codec_id 0: rawvideo
>  #dimensions 0: 102x109
>  #sar 0: 0/1
> -0,          0,          0,        1,    22236, 0x34490902
> +0,          0,          0,        1,    22236, 0x24634517
> diff --git a/tests/ref/fate/fitsdec-bitpix-64 b/tests/ref/fate/fitsdec-bitpix-64
> index 9febdd68f4..e50d5e029c 100644
> --- a/tests/ref/fate/fitsdec-bitpix-64
> +++ b/tests/ref/fate/fitsdec-bitpix-64
> @@ -3,4 +3,4 @@
>  #codec_id 0: rawvideo
>  #dimensions 0: 77x173
>  #sar 0: 0/1
> -0,          0,          0,        1,    26642, 0x0ad2a46a
> +0,          0,          0,        1,    26642, 0xa9eec634
> diff --git a/tests/ref/fate/fitsdec-blank_bitpix32 b/tests/ref/fate/fitsdec-blank_bitpix32
> index 184fd41c59..330d6710ca 100644
> --- a/tests/ref/fate/fitsdec-blank_bitpix32
> +++ b/tests/ref/fate/fitsdec-blank_bitpix32
> @@ -3,4 +3,4 @@
>  #codec_id 0: rawvideo
>  #dimensions 0: 256x256
>  #sar 0: 0/1
> -0,          0,          0,        1,   131072, 0x7fb22427
> +0,          0,          0,        1,   131072, 0x3ecd0739
> diff --git a/tests/ref/fate/fitsdec-ext_data_min_max b/tests/ref/fate/fitsdec-ext_data_min_max
> index 9009a4efb3..006d8d6250 100644
> --- a/tests/ref/fate/fitsdec-ext_data_min_max
> +++ b/tests/ref/fate/fitsdec-ext_data_min_max
> @@ -3,4 +3,4 @@
>  #codec_id 0: rawvideo
>  #dimensions 0: 512x512
>  #sar 0: 0/1
> -0,          0,          0,        1,   524288, 0xc327ed23
> +0,          0,          0,        1,   524288, 0x6567ecb3
> diff --git a/tests/ref/fate/fitsdec-gray b/tests/ref/fate/fitsdec-gray
> index 425b31fc0f..d080732452 100644
> --- a/tests/ref/fate/fitsdec-gray
> +++ b/tests/ref/fate/fitsdec-gray
> @@ -3,4 +3,4 @@
>  #codec_id 0: rawvideo
>  #dimensions 0: 128x128
>  #sar 0: 0/1
> -0,          0,          0,        1,    16384, 0xd788a2d2
> +0,          0,          0,        1,    16384, 0x353dbacd
> diff --git a/tests/ref/lavf/gray16be.fits b/tests/ref/lavf/gray16be.fits
> index 078d6c8678..058fa4ad19 100644
> --- a/tests/ref/lavf/gray16be.fits
> +++ b/tests/ref/lavf/gray16be.fits
> @@ -1,3 +1,3 @@
>  15e85a553bbd07783f92377ed369308b *tests/data/lavf/lavf.gray16be.fits
>  5184000 tests/data/lavf/lavf.gray16be.fits
> -tests/data/lavf/lavf.gray16be.fits CRC=0x8b840cff
> +tests/data/lavf/lavf.gray16be.fits CRC=0x8cdcbeb2
>
Michael Niedermayer Oct. 1, 2019, 4:03 p.m.
On Mon, Sep 30, 2019 at 10:30:59PM -0300, James Almer wrote:
> On 9/30/2019 1:30 PM, Michael Niedermayer wrote:
> > Fixes: -nan is outside the range of representable values of type 'unsigned short'
> 
> From lrint documentation:
> 
> "If x is a NaN or an infinity, or the rounded value is too large to be
> stored in a long (long long in the case of the ll* functions), then a
> domain error occurs, and the return value is unspecified."
> 
> So i don't know if using lrint is a good idea here.
> 
> > Fixes: 17769/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5678314672357376
> 
> Is the output of av_int2double/av_int2float or header.data_min NaN in
> that testcase? Wouldn't it be better to check that instead, and abort?

If you prefer we can do that (check every pixel for being NaN and if not
then within the range.)
I was just hesitating a bit as this is likely going to result in speed loss
And in the most strictest sense it is not preventing the error case as
floats dont gurantee that level of consistency ...

thanks

[...]
Michael Niedermayer Dec. 5, 2019, 5:46 p.m.
On Mon, Sep 30, 2019 at 10:30:59PM -0300, James Almer wrote:
> On 9/30/2019 1:30 PM, Michael Niedermayer wrote:
> > Fixes: -nan is outside the range of representable values of type 'unsigned short'
> 
> From lrint documentation:
> 
> "If x is a NaN or an infinity, or the rounded value is too large to be
> stored in a long (long long in the case of the ll* functions), then a
> domain error occurs, and the return value is unspecified."
> 
> So i don't know if using lrint is a good idea here.
> 
> > Fixes: 17769/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5678314672357376
> 
> Is the output of av_int2double/av_int2float or header.data_min NaN in
> that testcase? Wouldn't it be better to check that instead, and abort?

This patch also fixes the fate failures on 32bit x86 fitsdec-bitpix-64

I can add per pixel checks but i expect that not to fix fate
so i suggest to apply this as a fate fix first and then we can decide if we
need an additional fix for NaN

thx

[...]
James Almer Dec. 5, 2019, 5:51 p.m.
On 12/5/2019 2:46 PM, Michael Niedermayer wrote:
> On Mon, Sep 30, 2019 at 10:30:59PM -0300, James Almer wrote:
>> On 9/30/2019 1:30 PM, Michael Niedermayer wrote:
>>> Fixes: -nan is outside the range of representable values of type 'unsigned short'
>>
>> From lrint documentation:
>>
>> "If x is a NaN or an infinity, or the rounded value is too large to be
>> stored in a long (long long in the case of the ll* functions), then a
>> domain error occurs, and the return value is unspecified."
>>
>> So i don't know if using lrint is a good idea here.
>>
>>> Fixes: 17769/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5678314672357376
>>
>> Is the output of av_int2double/av_int2float or header.data_min NaN in
>> that testcase? Wouldn't it be better to check that instead, and abort?
> 
> This patch also fixes the fate failures on 32bit x86 fitsdec-bitpix-64
> 
> I can add per pixel checks but i expect that not to fix fate
> so i suggest to apply this as a fate fix first and then we can decide if we
> need an additional fix for NaN
> 
> thx

Fine by me.
Michael Niedermayer Dec. 5, 2019, 7:14 p.m.
On Thu, Dec 05, 2019 at 02:51:32PM -0300, James Almer wrote:
> On 12/5/2019 2:46 PM, Michael Niedermayer wrote:
> > On Mon, Sep 30, 2019 at 10:30:59PM -0300, James Almer wrote:
> >> On 9/30/2019 1:30 PM, Michael Niedermayer wrote:
> >>> Fixes: -nan is outside the range of representable values of type 'unsigned short'
> >>
> >> From lrint documentation:
> >>
> >> "If x is a NaN or an infinity, or the rounded value is too large to be
> >> stored in a long (long long in the case of the ll* functions), then a
> >> domain error occurs, and the return value is unspecified."
> >>
> >> So i don't know if using lrint is a good idea here.
> >>
> >>> Fixes: 17769/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5678314672357376
> >>
> >> Is the output of av_int2double/av_int2float or header.data_min NaN in
> >> that testcase? Wouldn't it be better to check that instead, and abort?
> > 
> > This patch also fixes the fate failures on 32bit x86 fitsdec-bitpix-64
> > 
> > I can add per pixel checks but i expect that not to fix fate
> > so i suggest to apply this as a fate fix first and then we can decide if we
> > need an additional fix for NaN
> > 
> > thx
> 
> Fine by me.

will apply

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/fitsdec.c b/libavcodec/fitsdec.c
index a20b8faf9e..2bb215943c 100644
--- a/libavcodec/fitsdec.c
+++ b/libavcodec/fitsdec.c
@@ -279,7 +279,7 @@  static int fits_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
             for (j = 0; j < avctx->width; j++) { \
                 t = rd; \
                 if (!header.blank_found || t != header.blank) { \
-                    *dst++ = ((t - header.data_min) * ((1 << (sizeof(type) * 8)) - 1)) * scale; \
+                    *dst++ = lrint(((t - header.data_min) * ((1 << (sizeof(type) * 8)) - 1)) * scale); \
                 } else { \
                     *dst++ = fitsctx->blank_val; \
                 } \
diff --git a/tests/ref/fate/fitsdec-bitpix-32 b/tests/ref/fate/fitsdec-bitpix-32
index 9bce361555..b3a51401d4 100644
--- a/tests/ref/fate/fitsdec-bitpix-32
+++ b/tests/ref/fate/fitsdec-bitpix-32
@@ -3,4 +3,4 @@ 
 #codec_id 0: rawvideo
 #dimensions 0: 102x109
 #sar 0: 0/1
-0,          0,          0,        1,    22236, 0x34490902
+0,          0,          0,        1,    22236, 0x24634517
diff --git a/tests/ref/fate/fitsdec-bitpix-64 b/tests/ref/fate/fitsdec-bitpix-64
index 9febdd68f4..e50d5e029c 100644
--- a/tests/ref/fate/fitsdec-bitpix-64
+++ b/tests/ref/fate/fitsdec-bitpix-64
@@ -3,4 +3,4 @@ 
 #codec_id 0: rawvideo
 #dimensions 0: 77x173
 #sar 0: 0/1
-0,          0,          0,        1,    26642, 0x0ad2a46a
+0,          0,          0,        1,    26642, 0xa9eec634
diff --git a/tests/ref/fate/fitsdec-blank_bitpix32 b/tests/ref/fate/fitsdec-blank_bitpix32
index 184fd41c59..330d6710ca 100644
--- a/tests/ref/fate/fitsdec-blank_bitpix32
+++ b/tests/ref/fate/fitsdec-blank_bitpix32
@@ -3,4 +3,4 @@ 
 #codec_id 0: rawvideo
 #dimensions 0: 256x256
 #sar 0: 0/1
-0,          0,          0,        1,   131072, 0x7fb22427
+0,          0,          0,        1,   131072, 0x3ecd0739
diff --git a/tests/ref/fate/fitsdec-ext_data_min_max b/tests/ref/fate/fitsdec-ext_data_min_max
index 9009a4efb3..006d8d6250 100644
--- a/tests/ref/fate/fitsdec-ext_data_min_max
+++ b/tests/ref/fate/fitsdec-ext_data_min_max
@@ -3,4 +3,4 @@ 
 #codec_id 0: rawvideo
 #dimensions 0: 512x512
 #sar 0: 0/1
-0,          0,          0,        1,   524288, 0xc327ed23
+0,          0,          0,        1,   524288, 0x6567ecb3
diff --git a/tests/ref/fate/fitsdec-gray b/tests/ref/fate/fitsdec-gray
index 425b31fc0f..d080732452 100644
--- a/tests/ref/fate/fitsdec-gray
+++ b/tests/ref/fate/fitsdec-gray
@@ -3,4 +3,4 @@ 
 #codec_id 0: rawvideo
 #dimensions 0: 128x128
 #sar 0: 0/1
-0,          0,          0,        1,    16384, 0xd788a2d2
+0,          0,          0,        1,    16384, 0x353dbacd
diff --git a/tests/ref/lavf/gray16be.fits b/tests/ref/lavf/gray16be.fits
index 078d6c8678..058fa4ad19 100644
--- a/tests/ref/lavf/gray16be.fits
+++ b/tests/ref/lavf/gray16be.fits
@@ -1,3 +1,3 @@ 
 15e85a553bbd07783f92377ed369308b *tests/data/lavf/lavf.gray16be.fits
 5184000 tests/data/lavf/lavf.gray16be.fits
-tests/data/lavf/lavf.gray16be.fits CRC=0x8b840cff
+tests/data/lavf/lavf.gray16be.fits CRC=0x8cdcbeb2