diff mbox

[FFmpeg-devel] smacker: limit recursion depth of smacker_decode_bigtree

Message ID a47f1cdf-4a57-32be-dc20-b52caeda2151@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Nov. 19, 2016, 11:44 p.m. UTC 
On 19.11.2016 23:34, Michael Niedermayer wrote:
> On Sat, Nov 19, 2016 at 05:27:19PM +0100, Andreas Cadhalpun wrote:
>> 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 ?

I don't think it's even possible to make this small enough for all cases,
as the stack size can be arbitrarily changed with 'ulimit -s'.

This value was chosen so that it works with the default stack size of 8 MB,
but if you think that's too much, it can be made smaller.

Attached is a variant reducing the 5000 to 500 and thus still working
with a stack size of only 0.8 MB.

Best regards,
Andreas

Comments

Andreas Cadhalpun Nov. 23, 2016, 12:22 a.m. UTC | #1 
On 20.11.2016 00:44, Andreas Cadhalpun wrote:
> On 19.11.2016 23:34, Michael Niedermayer wrote:
>> On Sat, Nov 19, 2016 at 05:27:19PM +0100, Andreas Cadhalpun wrote:
>>> 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 ?
> 
> I don't think it's even possible to make this small enough for all cases,
> as the stack size can be arbitrarily changed with 'ulimit -s'.
> 
> This value was chosen so that it works with the default stack size of 8 MB,
> but if you think that's too much, it can be made smaller.
> 
> Attached is a variant reducing the 5000 to 500 and thus still working
> with a stack size of only 0.8 MB.

I've now pushed this variant with reduced stack size needs.

Best regards,
Andreas
diff mbox

Patch

From 031676592dc15ea11fd677d37d094772478a16e0 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..2d20be9 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 > 500) { // 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