| Message ID | e8df3688-b6d8-2616-ffc3-ea56e4c587a9@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Nov 19, 2016 at 05:27:19PM +0100, Andreas Cadhalpun wrote: > On 19.11.2016 16:13, Michael Niedermayer wrote: > > On Sat, Nov 19, 2016 at 02:29:35PM +0100, Andreas Cadhalpun wrote: > >> This fixes segmentation faults due to stack-overflow caused by too deep > >> recursion. > >> > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > >> --- > >> libavcodec/smacker.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > > > breaks fate > > > > [smackvid @ 0x3586b80] size 31232 too large > > Input #0, smk, from 'fate/fate-suite//smacker/wetlogo.smk': > > Duration: 00:00:07.10, start: 0.000000, bitrate: 815 kb/s > > Stream #0:0: Video: smackvideo (SMK2 / 0x324B4D53), pal8, 320x200, 14.08 tbr, 14.08 tbn, 14.08 tbc > > Stream #0:1: Audio: smackaudio (SMKA / 0x414B4D53), 22050 Hz, mono, u8 > > [smackvid @ 0x358b860] size 31232 too large > > Stream mapping: > > Stream #0:0 -> #0:0 (smackvideo (smackvid) -> rawvideo (native)) > > Error while opening decoder for input stream #0:0 : Invalid data found when processing input > > make: *** [fate-smacker-video] Error 1 > > Then this has to be checked more directly, see attached patch. > > Best regards, > Andreas > > smacker.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > 91ae1418a6828a849e6239bd127e76d3a4ba700f 0001-smacker-limit-recursion-depth-of-smacker_decode_bigt.patch > From bcb4a860ff5ca987b5bcd99797704faaba6d23a2 Mon Sep 17 00:00:00 2001 > From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > Date: Sat, 19 Nov 2016 14:21:11 +0100 > Subject: [PATCH] smacker: limit recursion depth of smacker_decode_bigtree > > This fixes segmentation faults due to stack-overflow caused by too deep > recursion. > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavcodec/smacker.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c > index b8a0c55..be3914b 100644 > --- a/libavcodec/smacker.c > +++ b/libavcodec/smacker.c > @@ -129,8 +129,12 @@ static int smacker_decode_tree(GetBitContext *gb, HuffContext *hc, uint32_t pref > /** > * Decode header tree > */ > -static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc, DBCtx *ctx) > +static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc, DBCtx *ctx, int length) > { > + if(length > 5000) { // Larger length can cause segmentation faults due to too deep recursion. > + av_log(NULL, AV_LOG_ERROR, "length too long\n"); > + return AVERROR_INVALIDDATA; > + } are you sure this is not too large for some platforms ? patch ok otherwise [...]
On 20.11.2016 00:25, Luca Barbato wrote: > On 19/11/2016 17:27, Andreas Cadhalpun wrote: >> This fixes segmentation faults due to stack-overflow caused by too >> deep recursion. > > You shouldn't be able to use hc->current for the same purpose? That's what I tried first, but it doesn't work, since hc->current can be much larger than the actual recursion depth. > Another option is to write that function as a nested loop I guess. Possibly. Best regards, Andreas
From bcb4a860ff5ca987b5bcd99797704faaba6d23a2 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> Date: Sat, 19 Nov 2016 14:21:11 +0100 Subject: [PATCH] smacker: limit recursion depth of smacker_decode_bigtree This fixes segmentation faults due to stack-overflow caused by too deep recursion. Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavcodec/smacker.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c index b8a0c55..be3914b 100644 --- a/libavcodec/smacker.c +++ b/libavcodec/smacker.c @@ -129,8 +129,12 @@ static int smacker_decode_tree(GetBitContext *gb, HuffContext *hc, uint32_t pref /** * Decode header tree */ -static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc, DBCtx *ctx) +static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc, DBCtx *ctx, int length) { + if(length > 5000) { // Larger length can cause segmentation faults due to too deep recursion. + av_log(NULL, AV_LOG_ERROR, "length too long\n"); + return AVERROR_INVALIDDATA; + } if (hc->current + 1 >= hc->length) { av_log(NULL, AV_LOG_ERROR, "Tree size exceeded!\n"); return AVERROR_INVALIDDATA; @@ -159,12 +163,12 @@ static int smacker_decode_bigtree(GetBitContext *gb, HuffContext *hc, DBCtx *ctx int r = 0, r_new, t; t = hc->current++; - r = smacker_decode_bigtree(gb, hc, ctx); + r = smacker_decode_bigtree(gb, hc, ctx, length + 1); if(r < 0) return r; hc->values[t] = SMK_NODE | r; r++; - r_new = smacker_decode_bigtree(gb, hc, ctx); + r_new = smacker_decode_bigtree(gb, hc, ctx, length + 1); if (r_new < 0) return r_new; return r + r_new; @@ -275,7 +279,7 @@ static int smacker_decode_header_tree(SmackVContext *smk, GetBitContext *gb, int goto error; } - if (smacker_decode_bigtree(gb, &huff, &ctx) < 0) + if (smacker_decode_bigtree(gb, &huff, &ctx, 0) < 0) err = -1; skip_bits1(gb); if(ctx.last[0] == -1) ctx.last[0] = huff.current++; -- 2.10.2