[Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area
Nikita Pettik
korablev at tarantool.org
Thu Apr 2 17:01:00 MSK 2020
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.
More information about the Tarantool-patches
mailing list