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

[Reimar Döffinger]

On 16.07.2019, at 20:31, Michael Niedermayer <michael at niedermayer.cc> wrote:

> 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); \

I looked at the code, and now it makes even less sense to me.
First, why is that reported as an error at all?
Dividing by 0 is well defined for floating-point.
Next, your patch handles only one corner-case while not handling others.
For example, data_min and data_max can also be inf or NaN, which equally make no sense (and result in a division by NaN, which can hardly be better than a division by 0).
Next, bzero is applied to data_min and data_max which can cause precision issues, so it's a bit questionable but maybe non-trivial to fix completely.
However as data_max is never used but only the delta, most of these issues can be fixed much more thoroughly (and improve performance) by calculating and storing only that delta, and before applying bzero. Then delta can simply be overridden to 1 if it is fishy (not a normal or 0).
Of course there is a question if values above data_max are supposed to result in output > 1 or be clamped, but I guess that issue can be ignored.
As the code pays no particular attention to precision issue it would also be a question if calculating the inverse and use multiplications instead of divisions would make sense, but that admittedly is just an optimization.