[Tarantool-patches] [PATCH] json: fix silent change of global json settings
Olga Arkhangelskaia
arkholga at tarantool.org
Fri Feb 14 18:55:01 MSK 2020
Hi! Thanks a lot for the review. I did not make separate version
because there is any major change. I have fixed everything you mentioned
and moved the test case to the separate file.
You will find new diff below.
14.02.2020 2:17, Alexander Turenko пишет:
>> Issue:https://github.com/tarantool/tarantool/issues/4761
>> Branch:https://github.com/tarantool/tarantool/tree/OKriw/gh-4761-json.decode-silently-changes-config-when-used-with-config-settings
> Nice catch!
>
> The patch itself is quite straighforward and obviously okay. I have
> several comments around wording, formatting and the test case.
>
> I'll paste the commit from the updated branch to comment it inline.
>
>> commit ddc51e76469f86b29ba0c37c6e2911bbba569bdb
>> Author: Olga Arkhangelskaia <arkholga at tarantool.org>
>> Date: Wed Feb 5 14:05:25 2020 +0300
>>
>> json: fix silent change of global json settings
> 'global' is not precise term here: it can be arbitrary json
> (de)serializer instance (created by json.new()), not necessarily the
> default one. It is better to say 'instance options'. Something like
> "don't spoil instance options with per-call ones".
>
>>
>> When json.decode is used with 2 arguments, 2nd argument seeps out to global
>> json settings. Moreover, due to current serializer.cfg implementation it
> Same for 'global'.
>
>> remains invisible while checking settings by json.cfg. To prevent such
>> behaviour we stop writing to global serializer struct and use local one,
>> to get one-time action.
>> As was mention before json.cfg can not be trusted in this case, so to check that
>> everything remained unchanged we call decode twice with and without 2nd
>> argument.
> Didn't get the second paragraph. Is it about the test? We usually don't
> describe tests in commit messages, but rather provide comments in the
> code of a test when it is necessary.
>
> Please, mention the commit where the degradation occurs (see [2] for
> example).
>
>>
>> Closes #4761
> Several nits:
>
> * Fit a commit body within 72 symbols.
> * It is better to split paragraphs with an empty line.
>
> See [1] for formal rules and examples.
>
> [1]: https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/
> [2]: https://github.com/tarantool/tarantool/commit/ccacba28f813fb99fd9eaf07fb41bf604dd341bc
>
>> diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
>> index fadfc74ec..a6b36ff3d 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(40)
>> + test:plan(41)
>>
>> test:test("unsigned", common.test_unsigned, serializer)
>> test:test("signed", common.test_signed, serializer)
>> @@ -94,6 +94,11 @@ tap.test("json", function(test)
>> 'error: too many nested data structures')
>> test:is(serializer.cfg.decode_max_depth, orig_decode_max_depth,
>> 'global option remains unchanged')
>> + --
>> + -- gh-4761 json.decode silently changes global settings of json when called
>> + -- with 2d parameter
>> + --
> Several nits:
>
> * Separate the comment from the previous test case with an empty line.
> * Use a colon after 'gh-xxxx' to unify it with other descriptions within
> the file.
> * Fit a comment within 66 symbols (see [2]; it is for C, but in fact we
> apply the rule to Lua).
> * Typo: 2d -> 2nd.
> * Same as above for 'global'.
>
> [2]: https://www.tarantool.io/en/doc/1.10/dev_guide/c_style_guide/
>
>> + test:ok(pcall(serializer.decode,'{"1":{"b":{"c":1,"d":null}},"a":1}'))
> The test case should be as much independent from other as possible. Here
> it uses the previous one, which calls <json instance>.decode() with
> per-call options. Moreover, 'Tarantool Engineer Standard Operating
> Procedures' document now obligates a developer to add a test case for a
> bug fix within a separate file (see 'Writing tests' section) and the
> reason is mostly to push developers to guarantee test cases
> independence. Please, note that it also holds a certain naming policy.
>
> You can use '{"foo": "bar"}' json string and {decode_max_depth = 1}
> option to minimize the test case. It will be easier to read.
>
> test.ok() has four parameters: test object (the colon adds it),
> condition to check, message and extra data to show when the test case
> fails. Let's provide the message, because of two reasons:
>
> * It eases initial analyzing of a fail when it occurs and so generally
> recommended.
> * In case of fail the second return value of pcall() will be shown as
> the message and it looks as unintended effect.
>
> I think it would be good to provide a regression test of the same kind
> for json.encode().
>
>>
>> --
>> -- gh-3514: fix parsing integers with exponent in json
>> diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
>> index 3d25814f3..5925e7e6f 100644
>> --- a/third_party/lua-cjson/lua_cjson.c
>> +++ b/third_party/lua-cjson/lua_cjson.c
>> @@ -1004,13 +1004,22 @@ static int json_decode(lua_State *l)
>> luaL_argcheck(l, lua_gettop(l) == 2 || lua_gettop(l) == 1, 1,
>> "expected 1 or 2 arguments");
>>
>> + struct luaL_serializer *cfg = luaL_checkserializer(l);
>> + struct luaL_serializer user_cfg;
>> + /*
>> + * user_cfg is per-call local version of global cfg: it is
>> + * used if user passes custom options to :decode() method
>> + * as a separate arguments. In this case it is required
>> + * to avoid modifying global parameters. Life span of
>> + * user_cfg is restricted by the scope of :decode() so it
>> + * is enough to allocate it on the stack.
>> + */
> Nit: We usually provide a description before a field / function /
> variable, not after.
>
>> + json.cfg = cfg;
>> if (lua_gettop(l) == 2) {
>> - struct luaL_serializer *user_cfg = luaL_checkserializer(l);
>> - luaL_serializer_parse_options(l, user_cfg);
>> + user_cfg = *cfg;
> Technically it also copies triggers. I would rather use
> luaL_serializer_copy_options() and left a comment that triggers are left
> uninitialized intentionally: the code should not run them. It is better
> because of two reasons:
>
> * trigger_run() would segfault if it will be called on user_cfg somehow
> and it will explicitly shows that something is going in a wrong way
> (likely on tests during development). It is better than change other
> serializer options using a trigger.
> * It is more obvious what is going here (I guess that the problem that
> you fixed here appears purely because this assignment was considered
> as the mistake and a pointer assignment should be here).
>
>> + luaL_serializer_parse_options(l, &user_cfg);
>> lua_pop(l, 1);
>> - json.cfg = user_cfg;
>> - } else {
>> - json.cfg = luaL_checkserializer(l);
>> + json.cfg = &user_cfg;
>> }
>>
>> json.data = luaL_checklstring(l, 1, &json_len);
-Subject: [PATCH] json: fix silent change of global json settings
+Subject: [PATCH] json: don't spoil instance options with per-call
-When json.decode is used with 2 arguments, 2nd argument seeps out to
global
-json settings. Morover, due to current serialier.cfg implementation it
-remains invisible while checking settings by json.cfg. To prevent sucj
-behaviour we stop writing to global serializer struct and use local one,
-to get one-time action.
-As was mention before json.cfg can not be trusted in this case, so to
check that
-everything remained unchanged we call decode twice with and without 2nd
-argument.
+When json.decode is used with 2 arguments, 2nd argument seeps out to
the json
+configuration of the instance. Moreover, due to current serializer.cfg
+implementation it remains invisible while checking settings by json.cfg.
+
+This fixes commit 6508ddb ("json: fix stack-use-after-scope in
json_decode()").
-diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
-index fadfc74ec..a6b36ff3d 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(40)
-+ test:plan(41)
-
- test:test("unsigned", common.test_unsigned, serializer)
- test:test("signed", common.test_signed, serializer)
-@@ -94,6 +94,11 @@ tap.test("json", function(test)
- 'error: too many nested data structures')
- test:is(serializer.cfg.decode_max_depth, orig_decode_max_depth,
- 'global option remains unchanged')
++++ b/test/app-tap/gh-4761-fix-json-decode.test.lua
+@@ -0,0 +1,28 @@
++#!/usr/bin/env tarantool
++
++local tap = require('tap')
++tap.test("json", function(test)
++ local serializer = require('json')
++ test:plan(4)
++
+ --
-+ -- gh-4761 json.decode silently changes global settings of json
when called
-+ -- with 2d parameter
++ -- gh-4761: json.decode silently changes instances settings when
called
++ -- with 2nd parameter.
+ --
-+ test:ok(pcall(serializer.decode,'{"1":{"b":{"c":1,"d":null}},"a":1}'))
-
- --
- -- gh-3514: fix parsing integers with exponent in json
++ test:ok(not pcall(serializer.decode, '{"foo":{"bar": 1}}',
++ {decode_max_depth = 1}),
++ 'error: too many nested data structures')
++ test:ok(pcall(serializer.decode, '{"foo":{"bar": 1}}'),
++ 'json instance settings are unchanged')
++
++ --
++ -- Same check for json.encode.
++ --
++ local nan = 1/0
++ test:ok(not pcall(serializer.encode, {a = 1/0},
++ {encode_invalid_numbers = false}),
++ 'expected error with NaN encoding with .encode')
++ test:ok(pcall(serializer.encode, {a=nan}),
++ "json instance settings are unchanged")
++
++end)
diff --git a/third_party/lua-cjson/lua_cjson.c
b/third_party/lua-cjson/lua_cjson.c
-index 3d25814f3..f855cbd80 100644
+index 3d25814f3..8ff2ca89a 100644
--- a/third_party/lua-cjson/lua_cjson.c
+++ b/third_party/lua-cjson/lua_cjson.c
-@@ -1004,13 +1004,13 @@ static int json_decode(lua_State *l)
+@@ -1004,13 +1004,26 @@ static int json_decode(lua_State *l)
luaL_argcheck(l, lua_gettop(l) == 2 || lua_gettop(l) == 1, 1,
"expected 1 or 2 arguments");
+ struct luaL_serializer *cfg = luaL_checkserializer(l);
-+ struct luaL_serializer user_cfg = *cfg;
++
++ /* user_cfg is per-call local version of global cfg: it is
++ * used if user passes custom options to :decode() method
++ * as a separate arguments. In this case it is required
++ * to avoid modifying global parameters. Life span of
++ * user_cfg is restricted by the scope of :decode() so it
++ * is enough to allocate it on the stack. */
++ struct luaL_serializer user_cfg;
+ json.cfg = cfg;
if (lua_gettop(l) == 2) {
- struct luaL_serializer *user_cfg = luaL_checkserializer(l);
- luaL_serializer_parse_options(l, user_cfg);
-+ luaL_serializer_parse_options(l, &user_cfg);
- lua_pop(l, 1);
+- lua_pop(l, 1);
- json.cfg = user_cfg;
- } else {
More information about the Tarantool-patches
mailing list