On Thu, Jul 18, 2019 at 08:21:21AM +0200, Reimar Döffinger wrote: > > > On 16.07.2019, at 20:31, Michael Niedermayer <mich...@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 <mich...@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 <mich...@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. > 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. 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 Thnaks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are best at talking, realize last or never when they are wrong.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
