[FFmpeg-devel,V3] lavfi/nlmeans: Checking number precision when computing integral images

Submitted by Jun Zhao on March 7, 2019, 1:45 a.m.

Details

Message ID 1551923112-22459-1-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao March 7, 2019, 1:45 a.m.
From: Jun Zhao <barryjzhao@tencent.com>

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 <barryjzhao@tencent.com>
---
 libavfilter/vf_nlmeans.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

Comments

Michael Niedermayer March 7, 2019, 8:18 p.m.
On Thu, Mar 07, 2019 at 09:45:12AM +0800, Jun Zhao wrote:
> From: Jun Zhao <barryjzhao@tencent.com>
> 
> 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 <barryjzhao@tencent.com>
> ---
>  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 Niedermayer March 7, 2019, 8:27 p.m.
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 <barryjzhao@tencent.com>
> > 
> > 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 <barryjzhao@tencent.com>
> > ---
> >  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

[...]
mypopy@gmail.com March 8, 2019, 1:35 a.m.
On Fri, Mar 8, 2019 at 4:28 AM Michael Niedermayer <michael@niedermayer.cc>
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 <barryjzhao@tencent.com>
> > >
> > > 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 <barryjzhao@tencent.com>
> > > ---
> > >  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

Patch hide | download patch | download mbox

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,