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

Submitted by Jun Zhao on March 8, 2019, 8:14 a.m.

Details

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

Commit Message

Jun Zhao March 8, 2019, 8:14 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 |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

Comments

Paul B Mahol March 8, 2019, 9:18 a.m.
On 3/8/19, Jun Zhao <mypopydev@gmail.com> 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 |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c
> index dcb5a03..8d47f9d 100644
> --- a/libavfilter/vf_nlmeans.c
> +++ b/libavfilter/vf_nlmeans.c
> @@ -477,6 +477,17 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *in)
>      NLMeansContext *s = ctx->priv;
>      AVFilterLink *outlink = ctx->outputs[0];
>
> +    // accumulation of 8-bits uint_8 into 32-bits 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, so limit the image
> size
> +    if ((UINT32_MAX / (uint64_t)inlink->w) < (255 * (uint64_t)inlink->h)) {
> +        av_log(ctx, AV_LOG_ERROR,
> +               "image size (%d x %d) integral value may overflow.\n",
> +               inlink->w, inlink->h);
> +        av_frame_free(&in);
> +        return AVERROR(EINVAL);
> +    }
> +
>      AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
>      if (!out) {
>          av_frame_free(&in);

I see no point in this warning, if overflow is real issue should be
fixed instead of giving
pointless warning.
mypopy@gmail.com March 8, 2019, 9:38 a.m.
On Fri, Mar 8, 2019 at 5:26 PM Paul B Mahol <onemda@gmail.com> wrote:
>
> On 3/8/19, Jun Zhao <mypopydev@gmail.com> 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 |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> >
> > diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c
> > index dcb5a03..8d47f9d 100644
> > --- a/libavfilter/vf_nlmeans.c
> > +++ b/libavfilter/vf_nlmeans.c
> > @@ -477,6 +477,17 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> > *in)
> >      NLMeansContext *s = ctx->priv;
> >      AVFilterLink *outlink = ctx->outputs[0];
> >
> > +    // accumulation of 8-bits uint_8 into 32-bits 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, so limit the image
> > size
> > +    if ((UINT32_MAX / (uint64_t)inlink->w) < (255 * (uint64_t)inlink->h)) {
> > +        av_log(ctx, AV_LOG_ERROR,
> > +               "image size (%d x %d) integral value may overflow.\n",
> > +               inlink->w, inlink->h);
> > +        av_frame_free(&in);
> > +        return AVERROR(EINVAL);
> > +    }
> > +
> >      AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
> >      if (!out) {
> >          av_frame_free(&in);
>
> I see no point in this warning, if overflow is real issue should be
> fixed instead of giving
> pointless warning.
In fact, this is a potential overflow problems depend on image
value/width/height when calculating integral image(Summed-area_table
is the other name https://en.wikipedia.org/wiki/Summed-area_table),
this is the reason to limit the image size in this patch to avoid this
potential overflow problems, I don't know what's the mean for " should
be fixed instead of giving pointless warning.", can you give more
information for this? thx.
Paul B Mahol March 8, 2019, 10:13 a.m.
On 3/8/19, mypopy@gmail.com <mypopy@gmail.com> wrote:
> On Fri, Mar 8, 2019 at 5:26 PM Paul B Mahol <onemda@gmail.com> wrote:
>>
>> On 3/8/19, Jun Zhao <mypopydev@gmail.com> 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 |   11 +++++++++++
>> >  1 files changed, 11 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c
>> > index dcb5a03..8d47f9d 100644
>> > --- a/libavfilter/vf_nlmeans.c
>> > +++ b/libavfilter/vf_nlmeans.c
>> > @@ -477,6 +477,17 @@ static int filter_frame(AVFilterLink *inlink,
>> > AVFrame
>> > *in)
>> >      NLMeansContext *s = ctx->priv;
>> >      AVFilterLink *outlink = ctx->outputs[0];
>> >
>> > +    // accumulation of 8-bits uint_8 into 32-bits 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, so limit the
>> > image
>> > size
>> > +    if ((UINT32_MAX / (uint64_t)inlink->w) < (255 *
>> > (uint64_t)inlink->h)) {
>> > +        av_log(ctx, AV_LOG_ERROR,
>> > +               "image size (%d x %d) integral value may overflow.\n",
>> > +               inlink->w, inlink->h);
>> > +        av_frame_free(&in);
>> > +        return AVERROR(EINVAL);
>> > +    }
>> > +
>> >      AVFrame *out = ff_get_video_buffer(outlink, outlink->w,
>> > outlink->h);
>> >      if (!out) {
>> >          av_frame_free(&in);
>>
>> I see no point in this warning, if overflow is real issue should be
>> fixed instead of giving
>> pointless warning.
> In fact, this is a potential overflow problems depend on image
> value/width/height when calculating integral image(Summed-area_table
> is the other name https://en.wikipedia.org/wiki/Summed-area_table),
> this is the reason to limit the image size in this patch to avoid this
> potential overflow problems, I don't know what's the mean for " should
> be fixed instead of giving pointless warning.", can you give more
> information for this? thx.

Use uint64_t type.

Patch hide | download patch | download mbox

diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c
index dcb5a03..8d47f9d 100644
--- a/libavfilter/vf_nlmeans.c
+++ b/libavfilter/vf_nlmeans.c
@@ -477,6 +477,17 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *in)
     NLMeansContext *s = ctx->priv;
     AVFilterLink *outlink = ctx->outputs[0];
 
+    // accumulation of 8-bits uint_8 into 32-bits 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, so limit the image size
+    if ((UINT32_MAX / (uint64_t)inlink->w) < (255 * (uint64_t)inlink->h)) {
+        av_log(ctx, AV_LOG_ERROR,
+               "image size (%d x %d) integral value may overflow.\n",
+               inlink->w, inlink->h);
+        av_frame_free(&in);
+        return AVERROR(EINVAL);
+    }
+
     AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
     if (!out) {
         av_frame_free(&in);