[PATCH] vf_unsharp: extend/improve feedback for validity checks
Abort for invalid too big values, and exactly state why the input value is invalid. In particular, avoid out-of-buffer access with too big values. --- libavfilter/vf_unsharp.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-) diff --git a/libavfilter/vf_unsharp.c b/libavfilter/vf_unsharp.c index e41e76f..9c85e5d 100644 --- a/libavfilter/vf_unsharp.c +++ b/libavfilter/vf_unsharp.c @@ -135,19 +135,27 @@ static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque) UnsharpContext *unsharp = ctx->priv; int lmsize_x = 5, cmsize_x = 0; int lmsize_y = 5, cmsize_y = 0; + int val; double lamount = 1.0f, camount = 0.0f; if (args) sscanf(args, "%d:%d:%lf:%d:%d:%lf", &lmsize_x, &lmsize_y, &lamount, &cmsize_x, &cmsize_y, &camount); - if ((lamount && (lmsize_x < 2 || lmsize_y < 2)) || - (camount && (cmsize_x < 2 || cmsize_y < 2))) { - av_log(ctx, AV_LOG_ERROR, - "Invalid value <2 for lmsize_x:%d or lmsize_y:%d or cmsize_x:%d or cmsize_y:%d\n", - lmsize_x, lmsize_y, cmsize_x, cmsize_y); - return AVERROR(EINVAL); +#define CHECK_SIZE(lc, xy, lc_str) \ + val = lc##msize_##xy; \ + if (val < MATRIX_MIN_LINE_SIZE || val > MATRIX_MAX_LINE_SIZE) { \ + av_log(ctx, AV_LOG_ERROR, \ + "Invalid value '%d' for %s %s size, " \ + "must be >= %d and <= %d\n", \ + val, #lc_str, #xy, \ + MATRIX_MIN_LINE_SIZE, MATRIX_MAX_LINE_SIZE); \ + return AVERROR(EINVAL); \ } + CHECK_SIZE(l, x, luma); + CHECK_SIZE(l, y, luma); + CHECK_SIZE(c, x, chroma); + CHECK_SIZE(c, y, chroma); set_filter_param(&unsharp->luma, lmsize_x, lmsize_y, lamount); set_filter_param(&unsharp->chroma, cmsize_x, cmsize_y, camount); -- 1.7.2.5
On date Saturday 2011-08-13 01:11:49 +0200, Stefano Sabatini encoded:
Abort for invalid too big values, and exactly state why the input value is invalid.
In particular, avoid out-of-buffer access with too big values. --- libavfilter/vf_unsharp.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
Regarding the syntax, the mp=unsharp filter supports a format of the kind: [lc]XxY:AMOUNT while unsharp supports only (awkward) positional parameters. A better mixed solution would be to implement a syntax of the kind: [lc]=XxY+AMOUNT for example: l=5x5+0.3:c=3x3-2.0 while back-supporting the old syntax. -- FFmpeg = Freak Faithless Muttering Power Erroneous Gadget
On Sat, Aug 13, 2011 at 01:29:25AM +0200, Stefano Sabatini wrote:
On date Saturday 2011-08-13 01:11:49 +0200, Stefano Sabatini encoded:
Abort for invalid too big values, and exactly state why the input value is invalid.
In particular, avoid out-of-buffer access with too big values. --- libavfilter/vf_unsharp.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
Regarding the syntax, the mp=unsharp filter supports a format of the kind: [lc]XxY:AMOUNT
while unsharp supports only (awkward) positional parameters.
A better mixed solution would be to implement a syntax of the kind: [lc]=XxY+AMOUNT
for example: l=5x5+0.3:c=3x3-2.0
while back-supporting the old syntax.
the old code also seems to support l1:2 as a shorthand of l1x1:2 from looking at the code and iam not sure if using a = is a good idea, that makes unsharp=l=5x5+0.3 and to me the double = looks a bit odd but thats just my 2 cent [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 3 "Rare item" - "Common item with rare defect or maybe just a lie" "Professional" - "'Toy' made in china, not functional except as doorstop" "Experts will know" - "The seller hopes you are not an expert"
On date Sunday 2011-08-14 14:42:04 +0200, Michael Niedermayer encoded:
On Sat, Aug 13, 2011 at 01:29:25AM +0200, Stefano Sabatini wrote:
On date Saturday 2011-08-13 01:11:49 +0200, Stefano Sabatini encoded:
Abort for invalid too big values, and exactly state why the input value is invalid.
In particular, avoid out-of-buffer access with too big values. --- libavfilter/vf_unsharp.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
Regarding the syntax, the mp=unsharp filter supports a format of the kind: [lc]XxY:AMOUNT
while unsharp supports only (awkward) positional parameters.
A better mixed solution would be to implement a syntax of the kind: [lc]=XxY+AMOUNT
for example: l=5x5+0.3:c=3x3-2.0
while back-supporting the old syntax.
the old code also seems to support l1:2 as a shorthand of l1x1:2 from looking at the code
I can easily port this (undocumented) feature.
and iam not sure if using a = is a good idea, that makes unsharp=l=5x5+0.3 and to me the double = looks a bit odd but thats just my 2 cent
On the other hand this is how we handle options in other filters (FILTER=OPT_1=VAL_1:...:OPT_n=VAL_n), and we use ":" as option separator so I didn't want to make it different just in this case. Consider also that the mp=unsharp parsing code is not particularly robust and unambiguous (e.g. mp=unsharp=l31:1c13:1c23:12fooo is valid). I could add another backward compatibility syntax layer for the mp=unsharp syntax, but that would be imo overkill. -- FFmpeg = Freak Fast Maxi Prodigious Eccentric Gangster
On Sat, Aug 13, 2011 at 01:11:49AM +0200, Stefano Sabatini wrote:
Abort for invalid too big values, and exactly state why the input value is invalid.
In particular, avoid out-of-buffer access with too big values. --- libavfilter/vf_unsharp.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/libavfilter/vf_unsharp.c b/libavfilter/vf_unsharp.c index e41e76f..9c85e5d 100644 --- a/libavfilter/vf_unsharp.c +++ b/libavfilter/vf_unsharp.c @@ -135,19 +135,27 @@ static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque) UnsharpContext *unsharp = ctx->priv; int lmsize_x = 5, cmsize_x = 0; int lmsize_y = 5, cmsize_y = 0; + int val; double lamount = 1.0f, camount = 0.0f;
if (args) sscanf(args, "%d:%d:%lf:%d:%d:%lf", &lmsize_x, &lmsize_y, &lamount, &cmsize_x, &cmsize_y, &camount);
- if ((lamount && (lmsize_x < 2 || lmsize_y < 2)) || - (camount && (cmsize_x < 2 || cmsize_y < 2))) { - av_log(ctx, AV_LOG_ERROR, - "Invalid value <2 for lmsize_x:%d or lmsize_y:%d or cmsize_x:%d or cmsize_y:%d\n", - lmsize_x, lmsize_y, cmsize_x, cmsize_y); - return AVERROR(EINVAL); +#define CHECK_SIZE(lc, xy, lc_str) \ + val = lc##msize_##xy; \ + if (val < MATRIX_MIN_LINE_SIZE || val > MATRIX_MAX_LINE_SIZE) { \ + av_log(ctx, AV_LOG_ERROR, \ + "Invalid value '%d' for %s %s size, " \ + "must be >= %d and <= %d\n", \ + val, #lc_str, #xy, \ + MATRIX_MIN_LINE_SIZE, MATRIX_MAX_LINE_SIZE); \
The values look wrong, the arrays are not 2d to begin with in the way the port assumes so checking against a x of a x*x array isnt likely correct. The checks from the original libmpocodecs code should be used: uint32_t *SC[MAX_MATRIX_SIZE-1]; ... fp->msizeX = 1 | av_clip(fp->msizeX, MIN_MATRIX_SIZE, MAX_MATRIX_SIZE); fp->msizeY = 1 | av_clip(fp->msizeY, MIN_MATRIX_SIZE, MAX_MATRIX_SIZE); [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable
On Sun, Aug 14, 2011 at 02:38:52PM +0200, Michael Niedermayer wrote:
On Sat, Aug 13, 2011 at 01:11:49AM +0200, Stefano Sabatini wrote:
Abort for invalid too big values, and exactly state why the input value is invalid.
In particular, avoid out-of-buffer access with too big values. --- libavfilter/vf_unsharp.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/libavfilter/vf_unsharp.c b/libavfilter/vf_unsharp.c index e41e76f..9c85e5d 100644 --- a/libavfilter/vf_unsharp.c +++ b/libavfilter/vf_unsharp.c @@ -135,19 +135,27 @@ static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque) UnsharpContext *unsharp = ctx->priv; int lmsize_x = 5, cmsize_x = 0; int lmsize_y = 5, cmsize_y = 0; + int val; double lamount = 1.0f, camount = 0.0f;
if (args) sscanf(args, "%d:%d:%lf:%d:%d:%lf", &lmsize_x, &lmsize_y, &lamount, &cmsize_x, &cmsize_y, &camount);
- if ((lamount && (lmsize_x < 2 || lmsize_y < 2)) || - (camount && (cmsize_x < 2 || cmsize_y < 2))) { - av_log(ctx, AV_LOG_ERROR, - "Invalid value <2 for lmsize_x:%d or lmsize_y:%d or cmsize_x:%d or cmsize_y:%d\n", - lmsize_x, lmsize_y, cmsize_x, cmsize_y); - return AVERROR(EINVAL); +#define CHECK_SIZE(lc, xy, lc_str) \ + val = lc##msize_##xy; \ + if (val < MATRIX_MIN_LINE_SIZE || val > MATRIX_MAX_LINE_SIZE) { \ + av_log(ctx, AV_LOG_ERROR, \ + "Invalid value '%d' for %s %s size, " \ + "must be >= %d and <= %d\n", \ + val, #lc_str, #xy, \ + MATRIX_MIN_LINE_SIZE, MATRIX_MAX_LINE_SIZE); \
The values look wrong, the arrays are not 2d to begin with in the way the port assumes so checking against a x of a x*x array isnt likely correct.
The checks from the original libmpocodecs code should be used: uint32_t *SC[MAX_MATRIX_SIZE-1]; ... fp->msizeX = 1 | av_clip(fp->msizeX, MIN_MATRIX_SIZE, MAX_MATRIX_SIZE); fp->msizeY = 1 | av_clip(fp->msizeY, MIN_MATRIX_SIZE, MAX_MATRIX_SIZE);
to calrify, iam not suggesting that this be used litterally but the max/min values of it, which look more correct to me [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates
On date Sunday 2011-08-14 15:03:56 +0200, Michael Niedermayer encoded:
On Sun, Aug 14, 2011 at 02:38:52PM +0200, Michael Niedermayer wrote:
On Sat, Aug 13, 2011 at 01:11:49AM +0200, Stefano Sabatini wrote:
Abort for invalid too big values, and exactly state why the input value is invalid.
In particular, avoid out-of-buffer access with too big values. --- libavfilter/vf_unsharp.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/libavfilter/vf_unsharp.c b/libavfilter/vf_unsharp.c index e41e76f..9c85e5d 100644 --- a/libavfilter/vf_unsharp.c +++ b/libavfilter/vf_unsharp.c @@ -135,19 +135,27 @@ static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque) UnsharpContext *unsharp = ctx->priv; int lmsize_x = 5, cmsize_x = 0; int lmsize_y = 5, cmsize_y = 0; + int val; double lamount = 1.0f, camount = 0.0f;
if (args) sscanf(args, "%d:%d:%lf:%d:%d:%lf", &lmsize_x, &lmsize_y, &lamount, &cmsize_x, &cmsize_y, &camount);
- if ((lamount && (lmsize_x < 2 || lmsize_y < 2)) || - (camount && (cmsize_x < 2 || cmsize_y < 2))) { - av_log(ctx, AV_LOG_ERROR, - "Invalid value <2 for lmsize_x:%d or lmsize_y:%d or cmsize_x:%d or cmsize_y:%d\n", - lmsize_x, lmsize_y, cmsize_x, cmsize_y); - return AVERROR(EINVAL); +#define CHECK_SIZE(lc, xy, lc_str) \ + val = lc##msize_##xy; \ + if (val < MATRIX_MIN_LINE_SIZE || val > MATRIX_MAX_LINE_SIZE) { \ + av_log(ctx, AV_LOG_ERROR, \ + "Invalid value '%d' for %s %s size, " \ + "must be >= %d and <= %d\n", \ + val, #lc_str, #xy, \ + MATRIX_MIN_LINE_SIZE, MATRIX_MAX_LINE_SIZE); \
The values look wrong, the arrays are not 2d to begin with in the way the port assumes so checking against a x of a x*x array isnt likely correct.
The checks from the original libmpocodecs code should be used: uint32_t *SC[MAX_MATRIX_SIZE-1]; ... fp->msizeX = 1 | av_clip(fp->msizeX, MIN_MATRIX_SIZE, MAX_MATRIX_SIZE); fp->msizeY = 1 | av_clip(fp->msizeY, MIN_MATRIX_SIZE, MAX_MATRIX_SIZE);
to calrify, iam not suggesting that this be used litterally but the max/min values of it, which look more correct to me
Updated, perform the same checks of the original MP filter. -- FFmpeg = Fostering and Forgiving Monstrous Powerful Eretic Gnome
On Mon, Feb 06, 2012 at 03:41:41PM +0100, Stefano Sabatini wrote:
On date Sunday 2011-08-14 15:03:56 +0200, Michael Niedermayer encoded:
On Sun, Aug 14, 2011 at 02:38:52PM +0200, Michael Niedermayer wrote:
On Sat, Aug 13, 2011 at 01:11:49AM +0200, Stefano Sabatini wrote:
Abort for invalid too big values, and exactly state why the input value is invalid.
In particular, avoid out-of-buffer access with too big values. --- libavfilter/vf_unsharp.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/libavfilter/vf_unsharp.c b/libavfilter/vf_unsharp.c index e41e76f..9c85e5d 100644 --- a/libavfilter/vf_unsharp.c +++ b/libavfilter/vf_unsharp.c @@ -135,19 +135,27 @@ static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque) UnsharpContext *unsharp = ctx->priv; int lmsize_x = 5, cmsize_x = 0; int lmsize_y = 5, cmsize_y = 0; + int val; double lamount = 1.0f, camount = 0.0f;
if (args) sscanf(args, "%d:%d:%lf:%d:%d:%lf", &lmsize_x, &lmsize_y, &lamount, &cmsize_x, &cmsize_y, &camount);
- if ((lamount && (lmsize_x < 2 || lmsize_y < 2)) || - (camount && (cmsize_x < 2 || cmsize_y < 2))) { - av_log(ctx, AV_LOG_ERROR, - "Invalid value <2 for lmsize_x:%d or lmsize_y:%d or cmsize_x:%d or cmsize_y:%d\n", - lmsize_x, lmsize_y, cmsize_x, cmsize_y); - return AVERROR(EINVAL); +#define CHECK_SIZE(lc, xy, lc_str) \ + val = lc##msize_##xy; \ + if (val < MATRIX_MIN_LINE_SIZE || val > MATRIX_MAX_LINE_SIZE) { \ + av_log(ctx, AV_LOG_ERROR, \ + "Invalid value '%d' for %s %s size, " \ + "must be >= %d and <= %d\n", \ + val, #lc_str, #xy, \ + MATRIX_MIN_LINE_SIZE, MATRIX_MAX_LINE_SIZE); \
The values look wrong, the arrays are not 2d to begin with in the way the port assumes so checking against a x of a x*x array isnt likely correct.
The checks from the original libmpocodecs code should be used: uint32_t *SC[MAX_MATRIX_SIZE-1]; ... fp->msizeX = 1 | av_clip(fp->msizeX, MIN_MATRIX_SIZE, MAX_MATRIX_SIZE); fp->msizeY = 1 | av_clip(fp->msizeY, MIN_MATRIX_SIZE, MAX_MATRIX_SIZE);
to calrify, iam not suggesting that this be used litterally but the max/min values of it, which look more correct to me
Updated, perform the same checks of the original MP filter. -- FFmpeg = Fostering and Forgiving Monstrous Powerful Eretic Gnome
vf_unsharp.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) 907f8c2fc4cce4585477ebf98f97dcaa27560771 0002-vf_unsharp-extend-validity-checks-and-improve-feedba.patch From 740645238b359327c82ab4931c8eded92fbadb60 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-lala@poste.it> Date: Sat, 13 Aug 2011 01:08:48 +0200 Subject: [PATCH] vf_unsharp: extend validity checks, and improve feedback
Abort for invalid too big input values, and clearly state the failure reason.
In particular, avoid out-of-buffer access and crash with too big values.
LGTM [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin
On date Monday 2012-03-12 02:35:49 +0100, Michael Niedermayer encoded:
On Mon, Feb 06, 2012 at 03:41:41PM +0100, Stefano Sabatini wrote: [...]
vf_unsharp.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) 907f8c2fc4cce4585477ebf98f97dcaa27560771 0002-vf_unsharp-extend-validity-checks-and-improve-feedba.patch From 740645238b359327c82ab4931c8eded92fbadb60 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-lala@poste.it> Date: Sat, 13 Aug 2011 01:08:48 +0200 Subject: [PATCH] vf_unsharp: extend validity checks, and improve feedback
Abort for invalid too big input values, and clearly state the failure reason.
In particular, avoid out-of-buffer access and crash with too big values.
LGTM
Dropped in favor of incoming patch. -- FFmpeg = Frenzy and Faithful Minimal Puristic Extended Goblin
participants (3)
-
Michael Niedermayer -
Stefano Sabatini -
Stefano Sabatini