Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] Re: [PATCH v1 1/1] lua_cjson: fix segfault on recursive table encoding
       [not found] ` <20190904230023.l277z3ff76yrgjtu@tkn_work_nb>
@ 2019-09-07 11:50   ` Vladislav Shpilevoy
  0 siblings, 0 replies; 2+ messages in thread
From: Vladislav Shpilevoy @ 2019-09-07 11:50 UTC (permalink / raw)
  To: Alexander Turenko, Kirill Shcherbatov; +Cc: tarantool-patches, Kirill Yukhin

Hi! Thanks for the patch!

LGTM.

On 05/09/2019 01:00, Alexander Turenko wrote:
> The patch is okay, just one comment about the commit message.
> 
> Vlad, can you give a glance on that (just in case)?
> 
> The patch actually adds [this][1] increment for MP_ARRAY case.
> 
> [1]: https://github.com/mpx/lua-cjson/blob/e8972ac754788d3ef10a57a36016d6c3e85ba20d/lua_cjson.c#L691
> 
> WBR, Alexander Turenko.
> 
> On Tue, Sep 03, 2019 at 03:02:13PM +0300, Kirill Shcherbatov wrote:
>> The json.encode() used to cause a segfault in case of recursive
>> table:
>>   tbl = {}
>>   tbl[1] = tbl
>>   json.encode(tbl)
>>
>> Library doesn't test whether given object on Lua stack parsed
>> earlier, because it performs a lightweight in-depth traverse
>> of Lua stack. However it must stop when encode_max_depth is
>> reached (by design).
>>
>> Tarantool's lua_cjson implementation has a bug introduced during
>> porting original library: it doesn't handle some corner cases
>> checking this constant while original code doesn't have such
>> problem. This patch adopts author's approach to check
>> encode_max_depth limit. Thanks to handling this constraint
>> correctly the segfault no longer occurs.
> 
> I would clarify "it doesn't handle some corner cases" phrase: entering
> into a map correctly increases a current depth, while entering into an
> array didn't.
> 
>>
>> Closes #4366
>> ---
>> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4366-json-recursive-table-segfault
>> Issue: https://github.com/tarantool/tarantool/issues/4366
>>
>>  third_party/lua-cjson/lua_cjson.c |  6 +++---
>>  test/app-tap/json.test.lua        | 23 ++++++++++++++++++++++-
>>  2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
>> index 631c53e3f..be2416c69 100644
>> --- a/third_party/lua-cjson/lua_cjson.c
>> +++ b/third_party/lua-cjson/lua_cjson.c
>> @@ -364,7 +364,7 @@ static void json_append_object(lua_State *l, struct luaL_serializer *cfg,
>>      }
>>  
>>          /* table, key, value */
>> -    json_append_data(l, cfg, current_depth + 1, json);
>> +        json_append_data(l, cfg, current_depth, json);
>>          lua_pop(l, 1);
>>          /* table, key */
>>      }
>> @@ -402,13 +402,13 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg,
>>      case MP_MAP:
>>      if (current_depth >= cfg->encode_max_depth)
>>          return json_append_nil(cfg, json); /* Limit nested maps */
>> -    json_append_object(l, cfg, current_depth, json);
>> +    json_append_object(l, cfg, current_depth + 1, json);
>>      return;
>>      case MP_ARRAY:
>>      /* Array */
>>      if (current_depth >= cfg->encode_max_depth)
>>          return json_append_nil(cfg, json); /* Limit nested arrays */
>> -    json_append_array(l, cfg, current_depth, json, field.size);
>> +    json_append_array(l, cfg, current_depth + 1, json, field.size);
>>      return;
>>      case MP_EXT:
>>      /* handled by luaL_convertfield */
>> diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
>> index 7ec3b11c7..10f6f4ab2 100755
>> --- a/test/app-tap/json.test.lua
>> +++ b/test/app-tap/json.test.lua
>> @@ -22,7 +22,7 @@ end
>>  
>>  tap.test("json", function(test)
>>      local serializer = require('json')
>> -    test:plan(28)
>> +    test:plan(32)
>>  
>>      test:test("unsigned", common.test_unsigned, serializer)
>>      test:test("signed", common.test_signed, serializer)
>> @@ -100,4 +100,25 @@ tap.test("json", function(test)
>>      test:is(serializer.decode('{"var":2.0e+3}')["var"], 2000)
>>      test:is(serializer.decode('{"var":2.0e+3}')["var"], 2000)
>>      test:is(serializer.decode('{"var":2.0e+3}')["var"], 2000)
>> +
>> +    --
>> +    -- gh-4366: segmentation fault with recursive table
>> +    --
>> +    serializer.cfg({encode_max_depth = 2})
>> +    local rec1 = {}
>> +    rec1[1] = rec1
>> +    test:is(serializer.encode(rec1), '[[null]]')
>> +    local rec2 = {}
>> +    rec2['x'] = rec2
>> +    test:is(serializer.encode(rec2), '{"x":{"x":null}}')
>> +    local rec3 = {}
>> +    rec3[1] = rec3
>> +    rec3[2] = rec3
>> +    test:is(serializer.encode(rec3), '[[null,null],[null,null]]')
>> +    local rec4 = {}
>> +    rec4['a'] = rec4
>> +    rec4['b'] = rec4
>> +    test:is(serializer.encode(rec4),
>> +            '{"a":{"a":null,"b":null},"b":{"a":null,"b":null}}')
>> +    serializer.cfg({encode_max_depth = orig_encode_max_depth})
>>  end)
>> -- 
>> 2.22.1
>>

^ permalink raw reply	[flat|nested] 2+ messages in thread

* [tarantool-patches] Re: [PATCH v1 1/1] lua_cjson: fix segfault on recursive table encoding
  2019-08-19 12:37 [tarantool-patches] " Kirill Shcherbatov
@ 2019-09-09  8:03 ` Kirill Yukhin
  0 siblings, 0 replies; 2+ messages in thread
From: Kirill Yukhin @ 2019-09-09  8:03 UTC (permalink / raw)
  To: tarantool-patches; +Cc: alexander.turenko, Kirill Shcherbatov

Hello,

On 19 Aug 15:37, Kirill Shcherbatov wrote:
> The json.encode() used to cause a segfault in case of recursive
> table:
>   tbl = {}
>   tbl[1] = tbl
>   json.encode(tbl)
> 
> Library doesn't test whether given object on Lua stack parsed
> earlier, because it performs a lightweight in-depth traverse
> of Lua stack. However it must stop when encode_max_depth is
> reached (by design).
> 
> Tarantool's lua_cjson implementation has a bug introduced during
> porting original library: it doesn't handle some corner cases
> checking this constant while original code doesn't have such
> problem. This patch adopts author's approach to check
> encode_max_depth limit. Thanks to handling this constraint
> correctly the segfault no longer occurs.
> 
> Closes #4366
> ---
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4366-json-recursive-table-segfault
> Issue: https://github.com/tarantool/tarantool/issues/4366

I've checked your patch into 1.10, 2.1, 2.2 and master.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2019-09-09  8:03 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <2a755460ea51d369b19006f172a989222837f448.1567512084.git.kshcherbatov@tarantool.org>
     [not found] ` <20190904230023.l277z3ff76yrgjtu@tkn_work_nb>
2019-09-07 11:50   ` [tarantool-patches] Re: [PATCH v1 1/1] lua_cjson: fix segfault on recursive table encoding Vladislav Shpilevoy
2019-08-19 12:37 [tarantool-patches] " Kirill Shcherbatov
2019-09-09  8:03 ` [tarantool-patches] " Kirill Yukhin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox