Message ID | 20190930163027.11686-4-michael@niedermayer.cc |
---|---|
State | Accepted |
Commit | 37f31f4e509fe4ccc56a64edaa6fa3d95ee20466 |
Headers | show |
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 [...]
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
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(-)