Re: [FFmpeg-devel] [PATCH V3] lavfi/nlmeans: Checking number precision when computing integral images
On Fri, Mar 8, 2019 at 4:28 AM Michael Niedermayer wrote: > On Thu, Mar 07, 2019 at 09:18:42PM +0100, Michael Niedermayer wrote: > > On Thu, Mar 07, 2019 at 09:45:12AM +0800, Jun Zhao wrote: > > > From: Jun Zhao > > > > > > accumulation of 8-bits uint_8 (uint8_t *src) into 32-bits (uint32_t > *ii) > > > data type, it will have a risk of an integral value becoming larger > than > > > the 32-bits integer capacity and resulting in an integer overflow. For > > > this risk, add a checking with warning message. > > > > > > Signed-off-by: Jun Zhao > > > --- > > > libavfilter/vf_nlmeans.c |7 +++ > > > 1 files changed, 7 insertions(+), 0 deletions(-) > > > > > > diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c > > > index dcb5a03..9876aae 100644 > > > --- a/libavfilter/vf_nlmeans.c > > > +++ b/libavfilter/vf_nlmeans.c > > > @@ -236,6 +236,13 @@ static void compute_ssd_integral_image(const > NLMeansDSPContext *dsp, > > > // adjusted end x position of the safe area after width of the > safe area gets aligned > > > const int endx_safe = startx_safe + safe_pw; > > > > > > +// accumulation of 8-bits uint_8 (uint8_t *src) into 32-bits > (uint32_t *ii) > > > +// data type, it will have a risk of an integral value becoming > larger than > > > +// the 32-bits integer capacity and resulting in an integer > overflow. > > > +if ((UINT32_MAX / (uint64_t)w) < (255 * (uint64_t)h)) > > > +av_log(NULL, AV_LOG_WARNING, > > > + "image (%d x %d) integral value may overflow.\n", w > ,h); > > > > Printing a warning is not an adequate response for a integer overflow. > > Such thing is undefined behavior (in case signed of signed int) and must > > not occur. > > And if no signed ints are involved, while this is then not undefined it > still > gives the wrong result. Thats a bug, the bug should be fixed not a warning > be printed that the bug might be triggered > > Thanks > > Will give a stricter w * h check for this case, Tks ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH V3] lavfi/nlmeans: Checking number precision when computing integral images
On Thu, Mar 07, 2019 at 09:18:42PM +0100, Michael Niedermayer wrote: > On Thu, Mar 07, 2019 at 09:45:12AM +0800, Jun Zhao wrote: > > From: Jun Zhao > > > > accumulation of 8-bits uint_8 (uint8_t *src) into 32-bits (uint32_t *ii) > > data type, it will have a risk of an integral value becoming larger than > > the 32-bits integer capacity and resulting in an integer overflow. For > > this risk, add a checking with warning message. > > > > Signed-off-by: Jun Zhao > > --- > > libavfilter/vf_nlmeans.c |7 +++ > > 1 files changed, 7 insertions(+), 0 deletions(-) > > > > diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c > > index dcb5a03..9876aae 100644 > > --- a/libavfilter/vf_nlmeans.c > > +++ b/libavfilter/vf_nlmeans.c > > @@ -236,6 +236,13 @@ static void compute_ssd_integral_image(const > > NLMeansDSPContext *dsp, > > // adjusted end x position of the safe area after width of the safe > > area gets aligned > > const int endx_safe = startx_safe + safe_pw; > > > > +// accumulation of 8-bits uint_8 (uint8_t *src) into 32-bits (uint32_t > > *ii) > > +// data type, it will have a risk of an integral value becoming larger > > than > > +// the 32-bits integer capacity and resulting in an integer overflow. > > +if ((UINT32_MAX / (uint64_t)w) < (255 * (uint64_t)h)) > > +av_log(NULL, AV_LOG_WARNING, > > + "image (%d x %d) integral value may overflow.\n", w ,h); > > Printing a warning is not an adequate response for a integer overflow. > Such thing is undefined behavior (in case signed of signed int) and must > not occur. And if no signed ints are involved, while this is then not undefined it still gives the wrong result. Thats a bug, the bug should be fixed not a warning be printed that the bug might be triggered Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct answer. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH V3] lavfi/nlmeans: Checking number precision when computing integral images
On Thu, Mar 07, 2019 at 09:45:12AM +0800, Jun Zhao wrote: > From: Jun Zhao > > accumulation of 8-bits uint_8 (uint8_t *src) into 32-bits (uint32_t *ii) > data type, it will have a risk of an integral value becoming larger than > the 32-bits integer capacity and resulting in an integer overflow. For > this risk, add a checking with warning message. > > Signed-off-by: Jun Zhao > --- > libavfilter/vf_nlmeans.c |7 +++ > 1 files changed, 7 insertions(+), 0 deletions(-) > > diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c > index dcb5a03..9876aae 100644 > --- a/libavfilter/vf_nlmeans.c > +++ b/libavfilter/vf_nlmeans.c > @@ -236,6 +236,13 @@ static void compute_ssd_integral_image(const > NLMeansDSPContext *dsp, > // adjusted end x position of the safe area after width of the safe area > gets aligned > const int endx_safe = startx_safe + safe_pw; > > +// accumulation of 8-bits uint_8 (uint8_t *src) into 32-bits (uint32_t > *ii) > +// data type, it will have a risk of an integral value becoming larger > than > +// the 32-bits integer capacity and resulting in an integer overflow. > +if ((UINT32_MAX / (uint64_t)w) < (255 * (uint64_t)h)) > +av_log(NULL, AV_LOG_WARNING, > + "image (%d x %d) integral value may overflow.\n", w ,h); Printing a warning is not an adequate response for a integer overflow. Such thing is undefined behavior (in case signed of signed int) and must not occur. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have never wished to cater to the crowd; for what I know they do not approve, and what they approve I do not know. -- Epicurus signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH V3] lavfi/nlmeans: Checking number precision when computing integral images
From: Jun Zhao accumulation of 8-bits uint_8 (uint8_t *src) into 32-bits (uint32_t *ii) data type, it will have a risk of an integral value becoming larger than the 32-bits integer capacity and resulting in an integer overflow. For this risk, add a checking with warning message. Signed-off-by: Jun Zhao --- libavfilter/vf_nlmeans.c |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c index dcb5a03..9876aae 100644 --- a/libavfilter/vf_nlmeans.c +++ b/libavfilter/vf_nlmeans.c @@ -236,6 +236,13 @@ static void compute_ssd_integral_image(const NLMeansDSPContext *dsp, // adjusted end x position of the safe area after width of the safe area gets aligned const int endx_safe = startx_safe + safe_pw; +// accumulation of 8-bits uint_8 (uint8_t *src) into 32-bits (uint32_t *ii) +// data type, it will have a risk of an integral value becoming larger than +// the 32-bits integer capacity and resulting in an integer overflow. +if ((UINT32_MAX / (uint64_t)w) < (255 * (uint64_t)h)) +av_log(NULL, AV_LOG_WARNING, + "image (%d x %d) integral value may overflow.\n", w ,h); + // top part where only one of s1 and s2 is still readable, or none at all compute_unsafe_ssd_integral_image(ii, ii_linesize_32, 0, 0, -- 1.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
