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

[Reimar Döffinger]

On 18.07.2019, at 12:54, Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Thu, Jul 18, 2019 at 08:21:21AM +0200, Reimar Döffinger wrote:
>> 
>> 
>> 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.
> 
> at least at the point where its stored in an integer it becomes painfull
> to the compiler to find a way to do it.

Hm, I am not quite following. The division results in inf or nan, and those get converted to integer the usual way (not sure how well that is defined in C, but it's not a division by 0 error).

> Iam not sure if inf is a problem (from a very quick look) that would get
> divided to 0 i guess nan would be an issue, i didnt think of this, i will 
> redo this and post a better patch

inf / inf = nan
From my point of view if 0.0 / 0.0 is considered an issue that seems like it should apply for inf as well.
When it comes to actually correct code, there might also be the question what to do in that case.
Could be considered "bad data/normalization range, just skip the whole scaling".
Or could define it like OpenGL (and other) normalization operations:
-inf becomes -1, inf becomes 1 all else 0. But that would need a special code case, and I guess it's not worth it.