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

Submitted by Jun Zhao on March 6, 2019, 3:18 a.m.

Details

Message ID 1551842307-3607-3-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao March 6, 2019, 3:18 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

Carl Eugen Hoyos March 6, 2019, 7:54 a.m.
2019-03-06 4:18 GMT+01:00, Jun Zhao <mypopydev@gmail.com>:
> 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..31c8304 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 ((w * h * UINT8_MAX) > UINT32_MAX)

I don't think UINT8_MAX increases readability and I suspect
this should contain "UINT32_MAX / (w*h)" or similar on
one side.

> +        av_log(NULL, AV_LOG_WARNING,
> +               "image (%d x %d) integral value maybe overflow.\n", w ,h);

may overflow?

Carl Eugen
mypopy@gmail.com March 6, 2019, 11:31 a.m.
On Wed, Mar 6, 2019 at 3:55 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2019-03-06 4:18 GMT+01:00, Jun Zhao <mypopydev@gmail.com>:
> > 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..31c8304 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 ((w * h * UINT8_MAX) > UINT32_MAX)
>
> I don't think UINT8_MAX increases readability and I suspect
> this should contain "UINT32_MAX / (w*h)" or similar on
> one side.
>
You means like: UINT32_MAX/w < (UINT8_MAX * h) ?
> > +        av_log(NULL, AV_LOG_WARNING,
> > +               "image (%d x %d) integral value maybe overflow.\n", w ,h);
>
> may overflow?
Will update the warning message as the suggestion.Tks
Carl Eugen Hoyos March 6, 2019, 1:15 p.m.
2019-03-06 12:31 GMT+01:00, mypopy@gmail.com <mypopy@gmail.com>:
> On Wed, Mar 6, 2019 at 3:55 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>> 2019-03-06 4:18 GMT+01:00, Jun Zhao <mypopydev@gmail.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.
>> > +    if ((w * h * UINT8_MAX) > UINT32_MAX)
>>
>> I don't think UINT8_MAX increases readability and I suspect
>> this should contain "UINT32_MAX / (w*h)" or similar on
>> one side.
>>
> You means like: UINT32_MAX/w < (UINT8_MAX * h) ?

Actually: (UINT32_MAX / w < 255 * h)
(But that may only be me)

Carl Eugen
mypopy@gmail.com March 7, 2019, 1:47 a.m.
On Wed, Mar 6, 2019 at 9:15 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2019-03-06 12:31 GMT+01:00, mypopy@gmail.com <mypopy@gmail.com>:
> > On Wed, Mar 6, 2019 at 3:55 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
wrote:
> >>
> >> 2019-03-06 4:18 GMT+01:00, Jun Zhao <mypopydev@gmail.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.
> >> > +    if ((w * h * UINT8_MAX) > UINT32_MAX)
> >>
> >> I don't think UINT8_MAX increases readability and I suspect
> >> this should contain "UINT32_MAX / (w*h)" or similar on
> >> one side.
> >>
> > You means like: UINT32_MAX/w < (UINT8_MAX * h) ?
>
> Actually: (UINT32_MAX / w < 255 * h)
> (But that may only be me)
>
Update V3 patch for this check in https://patchwork.ffmpeg.org/patch/12230/,
Tks

Patch hide | download patch | download mbox

diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c
index dcb5a03..31c8304 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 ((w * h * UINT8_MAX) > UINT32_MAX)
+        av_log(NULL, AV_LOG_WARNING,
+               "image (%d x %d) integral value maybe 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,