[FFmpeg-devel] [PATCH 2/2] avcodec/fitsdec: Prevent division by 0 with huge data_max

[Michael Niedermayer]

On Tue, Jul 16, 2019 at 08:34:14AM +0200, Reimar Döffinger wrote:
> On 16.07.2019, at 00:50, Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > Fixes: division by 0
> > Fixes: 15657/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FITS_fuzzer-5738154838982656
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > ---
> > libavcodec/fitsdec.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/fitsdec.c b/libavcodec/fitsdec.c
> > index 4f452422ef..fe941f199d 100644
> > --- a/libavcodec/fitsdec.c
> > +++ b/libavcodec/fitsdec.c
> > @@ -174,7 +174,7 @@ static int fits_read_header(AVCodecContext *avctx, const uint8_t **ptr, FITSHead
> >             return AVERROR_INVALIDDATA;
> >         }
> >         av_log(avctx, AV_LOG_WARNING, "data min/max indicates a blank image\n");
> > -        header->data_max ++;
> > +        header->data_max += fabs(header->data_max) / 10000000 + 1;
> 
> This is really non-obvious, both by itself, in where/how it causes the division by 0 and that the error here isn't worse than the division by 0 for example, and why this constant was chosen.

division by 0 occured in:
*dst++ = ((t - header.data_min) * ((1 << (sizeof(type) * 8)) - 1)) / (header.data_max - header.data_min); \


> Also why a division and not a multiply by the inverse?

no reason, this could be changed


> Why not * (1.0f / (1 << 24)) for example, which for single-precision IEEE I think should result in exactly 1 ULP (well, possibly 2 with rounding) increments?

the division by 0 could occur with files which contain only one color. Or
otherwise corrupted / odd files.
what the code tries to do is to do a reasonable small change away from division
by 0. 
The way the whole implementation of fits is done is to scale the input range
to the output range. If the input is 0 as it can be on a constant color input
this hits a singularity. As this is basically a 0/0 anything could be output
and would be equally wrong so the exact value like / 10000000 or other dont
really matter.
For these values to matter, first how the decoder interprets data would have
to be changed:
" Also to interpret data, values are linearly scaled using min-max scaling but not RGB images."



> Why is this even using floating-point? And why not double-precision at least?


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190716/968d2fb7/attachment.sig>