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. > 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.
ill post a different patch which incorprorates some ideas from this thread thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Take away the freedom of one citizen and you will be jailed, take away the freedom of all citizens and you will be congratulated by your peers in Parliament.
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".
