From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 06C0F24663 for ; Sat, 7 Sep 2019 07:47:15 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ku6WqoNECuKm for ; Sat, 7 Sep 2019 07:47:14 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5DB6024661 for ; Sat, 7 Sep 2019 07:47:14 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] lua_cjson: fix segfault on recursive table encoding References: <2a755460ea51d369b19006f172a989222837f448.1567512084.git.kshcherbatov@tarantool.org> <20190904230023.l277z3ff76yrgjtu@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: <454d44cb-d103-0dcd-5db4-3cd7ff6a50d4@tarantool.org> Date: Sat, 7 Sep 2019 13:50:48 +0200 MIME-Version: 1.0 In-Reply-To: <20190904230023.l277z3ff76yrgjtu@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Alexander Turenko , Kirill Shcherbatov Cc: tarantool-patches@freelists.org, 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 >>