From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CB6C74696C3 for ; Thu, 2 Apr 2020 17:01:01 +0300 (MSK) Date: Thu, 2 Apr 2020 14:01:00 +0000 From: Nikita Pettik Message-ID: <20200402140100.GA30923@tarantool.org> References: <3bb7c695b34356137897b953281543c46fe3bafa.1585097339.git.korablev@tarantool.org> <20200401162618.GD26447@tarantool.org> <20200401222456.GA27659@tarantool.org> <64be3c70-4ec9-0743-0b6d-e7453239db4a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <64be3c70-4ec9-0743-0b6d-e7453239db4a@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 02 Apr 02:29, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > See 6 comments below. > > > diff --git a/src/box/xrow.c b/src/box/xrow.c > > index 28adbdc1f..f33226dc5 100644 > > --- a/src/box/xrow.c > > +++ b/src/box/xrow.c > > @@ -1070,19 +1083,62 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len, > > return 0; > > } > > > > +static int > > +iproto_decode_error_stack(const char **pos) > > +{ > > + const char *reason = NULL; > > + static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\ > > + "expected to be larger than error message max length"); > > + /* > > + * Erase previously set diag errors. It is required since > > + * box_error_add() does not replace previous errors. > > + */ > > + box_error_clear(); > > + assert(mp_typeof(**pos) == MP_ARRAY); > > 1. Better keep this check here instead of xrow_decode_error(). It looks > strange, when iproto_decode_error_stack() validates everything except > the top header, which is somewhy expected to be validated by the > caller. Ok: diff --git a/src/box/xrow.c b/src/box/xrow.c index f33226dc5..680f9329a 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -1094,7 +1094,8 @@ iproto_decode_error_stack(const char **pos) * box_error_add() does not replace previous errors. */ box_error_clear(); - assert(mp_typeof(**pos) == MP_ARRAY); + if (mp_typeof(**pos) != MP_ARRAY) + return -1; uint32_t stack_sz = mp_decode_array(pos); for (uint32_t i = 0; i < stack_sz; i++) { uint32_t code = 0; @@ -1163,8 +1164,6 @@ xrow_decode_error(struct xrow_header *row) snprintf(error, sizeof(error), "%.*s", len, str); box_error_set(__FILE__, __LINE__, code, error); } else if (key == IPROTO_ERROR_STACK) { - if (mp_typeof(*pos) != MP_ARRAY) - goto error; if (iproto_decode_error_stack(&pos) != 0) goto error; } else { > > + uint32_t stack_sz = mp_decode_array(pos); > > + for (uint32_t i = 0; i < stack_sz; i++) { > > + uint32_t code = 0; > > + if (mp_typeof(**pos) != MP_MAP) > > + return -1; > > + uint32_t map_sz = mp_decode_map(pos); > > + for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) { > > + if (mp_typeof(**pos) != MP_UINT) > > + return -1; > > + uint8_t key = mp_decode_uint(pos); > > 2. If I will send value 0xffffff01, it will be considered valid. > Better save it into uint64_t, since mp_decode_uint() returns this > type. In that case all will be fine. > > The same with code. Better get the code as uint64_t and compare > with UINT32_MAX. Because ClientError has uint32_t error code type. diff --git a/src/box/xrow.c b/src/box/xrow.c index f33226dc5..323dc8f4d 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -1094,17 +1094,18 @@ iproto_decode_error_stack(const char **pos) uint32_t stack_sz = mp_decode_array(pos); for (uint32_t i = 0; i < stack_sz; i++) { - uint32_t code = 0; + uint64_t code = 0; if (mp_typeof(**pos) != MP_MAP) return -1; uint32_t map_sz = mp_decode_map(pos); for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) { if (mp_typeof(**pos) != MP_UINT) return -1; - uint8_t key = mp_decode_uint(pos); + uint32_t key = mp_decode_uint(pos); if (key == IPROTO_ERROR_CODE) { if (mp_typeof(**pos) != MP_UINT) return -1; > > void > > xrow_decode_error(struct xrow_header *row) > > { > > uint32_t code = row->type & (IPROTO_TYPE_ERROR - 1); > > > > char error[DIAG_ERRMSG_MAX] = { 0 }; > > - const char *pos; > > + const char *pos, *end; > > uint32_t map_size; > > > > if (row->bodycnt == 0) > > goto error; > > pos = (char *) row->body[0].iov_base; > > - if (mp_check(&pos, pos + row->body[0].iov_len)) > > + end = pos + row->body[0].iov_len; > > + if (mp_check(&pos, end)) > > goto error; > > 3. This hunk is clearly an artifact from the old patch > version. Dropped it and nothing changed. Yes, sorry. Reverted change: @@ -1131,14 +1132,13 @@ xrow_decode_error(struct xrow_header *row) uint32_t code = row->type & (IPROTO_TYPE_ERROR - 1); char error[DIAG_ERRMSG_MAX] = { 0 }; - const char *pos, *end; + const char *pos; uint32_t map_size; if (row->bodycnt == 0) goto error; pos = (char *) row->body[0].iov_base; - end = pos + row->body[0].iov_len; - if (mp_check(&pos, end)) + if (mp_check(&pos, pos + row->body[0].iov_len)) goto error; pos = (char *) row->body[0].iov_base; > > diff --git a/test/box/iproto.result b/test/box/iproto.result > > new file mode 100644 > > index 000000000..b6dc7ed4c > > --- /dev/null > > +++ b/test/box/iproto.result > > @@ -0,0 +1,142 @@ > > +--- > > +... > > +-- gh-1148: test capabilities of stacked diagnostics bypassing net.box. > > +-- > > +test_run:cmd("setopt delimiter ';'") > > +--- > > +- true > > +... > > +stack_err = function() > > + local e1 = box.error.new({code = 111, reason = "e1"}) > > + local e2 = box.error.new({code = 111, reason = "e2"}) > > + local e3 = box.error.new({code = 111, reason = "e3"}) > > + assert(e1 ~= nil) > > + e2:set_prev(e1) > > + assert(e2.prev == e1) > > + e3:set_prev(e2) > > + box.error(e3) > > +end; > > +--- > > +... > > +test_run:cmd("setopt delimiter ''"); > > +--- > > +- true > > +... > > +box.schema.user.grant('guest', 'read, write, execute', 'universe') > > 4. Useful command, easy to remember: > > box.schema.user.grant/revoke('guest', 'super') > > Is shorter, and has basically the same effect + a few more permissions. > In case you like me look for this 'grant' command in other test files to > copy paste it every time. Up to you what to use. JFYI. Thx, will keep it in mind. > > diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc > > index 68a334239..954f22f16 100644 > > --- a/test/unit/xrow.cc > > +++ b/test/unit/xrow.cc > > 5. This test crashes on travis and locally on my machine. Probably > a typo somewhere. Yep, accidentally forgot to update result file. Fixed. > I also added a test for the uint64 vs uint8 result of mp_decode_uint(). > See it below and pushed on top of the branch as a separate commit. Nice, thanks, squashed with some changes: @@ -372,6 +375,26 @@ test_xrow_error_stack_decode() isnt(last, NULL, "xrow_decode succeed: diag has been set"); is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: " "stack's map wrong value type"); + + /* Bad key in the packet. */ + pos = mp_encode_map(buffer, 1); + pos = mp_encode_uint(pos, IPROTO_ERROR_STACK); + pos = mp_encode_array(pos, 1); + pos = mp_encode_map(pos, 2); + pos = mp_encode_uint(pos, 0xff000000 | IPROTO_ERROR_CODE); + pos = mp_encode_uint(pos, 159); + pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE); + pos = mp_encode_str(pos, "test msg", strlen("test msg")); + header.body[0].iov_base = buffer; + header.body[0].iov_len = pos - buffer; + + diag_clear(diag_get()); + xrow_decode_error(&header); + last = diag_last_error(diag_get()); + isnt(last, NULL, "xrow_decode succeed: diag has been set"); + is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode corrupted stack: " + "stack's map wrong key"); + check_plan(); } Diag won't be empty since error will be set anyway - with default (i.e. wrong) error code (0), but correct message. I've also found one more bug: box_error_add() was called in the wrong place. As a result, number of errors in stack might be doubled: @@ -1119,8 +1120,8 @@ iproto_decode_error_stack(const char **pos) mp_next(pos); continue; } - box_error_add(__FILE__, __LINE__, code, reason); } + box_error_add(__FILE__, __LINE__, code, reason); } return 0; } Added another one test case covering this change: @@ -295,12 +295,15 @@ test_xrow_error_stack_decode() isnt(last, NULL, "xrow_decode succeed: 'cause' is present in stack") is(strcmp(last->errmsg, "e1"), 0, "xrow_decode succeed: " "stack has been parsed"); + last = last->cause; + is(last, NULL, "xrow_decode succeed: only two errors in the stack") /* * Let's try decode broken stack. Variations: > ==================== > diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc > index 954f22f16..1449d49a5 100644 > --- a/test/unit/xrow.cc > +++ b/test/unit/xrow.cc > @@ -255,7 +255,7 @@ error_stack_entry_encode(char *pos, const char *err_str) > void > test_xrow_error_stack_decode() > { > - plan(14); > + plan(15); > char buffer[2048]; > /* > * To start with, let's test the simplest and obsolete > @@ -372,6 +372,24 @@ test_xrow_error_stack_decode() > isnt(last, NULL, "xrow_decode succeed: diag has been set"); > is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: " > "stack's map wrong value type"); > + > + /* Bad key in the packet. */ > + diag_clear(diag_get()); > + pos = mp_encode_map(buffer, 1); > + pos = mp_encode_uint(pos, IPROTO_ERROR_STACK); > + pos = mp_encode_array(pos, 1); > + pos = mp_encode_map(pos, 2); > + pos = mp_encode_uint(pos, 0xff000000 | IPROTO_ERROR_CODE); > + pos = mp_encode_uint(pos, 159); > + pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE); > + pos = mp_encode_str(pos, "", 0); > + header.body[0].iov_base = buffer; > + header.body[0].iov_len = pos - buffer; > + xrow_decode_error(&header); > + ok(diag_is_empty(diag_get()), > + "xrow_decode failed: bad error object key"); > + last = diag_last_error(diag_get()); > + > check_plan(); > } > ==================== > > On 02/04/2020 00:24, Nikita Pettik wrote: > > On 01 Apr 16:26, Nikita Pettik wrote: > >> @@ -1169,7 +1166,7 @@ xrow_decode_error(struct xrow_header *row) > >> } else if (key == IPROTO_ERROR_STACK ) { > >> if (mp_check_array(pos, end) != 0) > >> goto error; > >> - if (iproto_decode_error_stack(&pos, end) != 0) > >> + if (iproto_decode_error_stack(&pos) != 0) > >> continue; > >> } else { > >> mp_next(&pos); > >> > >>> I don't see any tests on that. Are there some? Maybe at least > >>> unit tests, in xrow.cc? Although a proper test would try to > >>> send an error stack through applier. Because xrow_decode_error() > >>> is used only there. > >> > >> I'm a bit new to replication tests. I've been working on it. > >> Could you please review the rest of changes so far? > > 6. I tried to find how to send a stacked error to applier. This > should be done from relay. And seems like it is impossible to do > from relay. Simply because there are no any user defined code to > create stacked errors, and because internals of relay don't use > stacked errors so far. Probably an error injection could help to > create an artificial error, but I think the unit test should be > enough.