| Message ID | 1552032852-18731-1-git-send-email-mypopydev@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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);