[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 New
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

[...]

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