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

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(-)

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
>

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

[...]

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

[...]

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.

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

[...]