[Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area
Nikita Pettik
korablev at tarantool.org
Fri Apr 3 05:16:47 MSK 2020
On 03 Apr 00:20, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
> See 2 comments below.
>
> > diff --git a/src/box/error.cc b/src/box/error.cc
> > index 8e77c2e9e..897aa9261 100644
> > --- a/src/box/error.cc
> > +++ b/src/box/error.cc
> > @@ -102,6 +102,23 @@ box_error_new(const char *file, unsigned line, uint32_t code,
> > return e;
> > }
> >
> > +int
> > +box_error_add(const char *file, unsigned line, uint32_t code,
> > + const char *fmt, ...)
> > +{
> > + struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> > + ClientError *client_error = type_cast(ClientError, e);
> > + if (client_error) {
> > + client_error->m_errcode = code;
> > + va_list ap;
> > + va_start(ap, fmt);
> > + error_vformat_msg(e, fmt, ap);
> > + va_end(ap);
> > + }
> > + diag_add_error(&fiber()->diag, e);
> > + return -1;
>
> 1. Why do we return -1 instead of the new error object?
> box_error_new() returns an error, and this function does not.
> Seems inconsistent.
box_error_new() is supposed to return new error according to
its name (to be more precise - '_new' suffix). It allocates and
fills in error struct. On the other hand, box_error_add() creates
error and adds it to the diagnostic area. It is simply meaningless
to return error, which is already set to diag.
> >>> 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
> > @@ -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.
>
> 2. I added box_error_code() check to ensure this.
>
> But more importantly that the original bug I was referring to
> still is here. About overflows and integer truncation. I fixed it
> and added tests. See them below and on the branch in a separate
> commit.
Oops, seems like I forgot to pop diff out from stash...
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index 9d30bcaf9..be026a43c 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -1105,11 +1105,13 @@ iproto_decode_error_stack(const char **pos)
> for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
> if (mp_typeof(**pos) != MP_UINT)
> return -1;
> - uint32_t key = mp_decode_uint(pos);
> + uint64_t key = mp_decode_uint(pos);
> if (key == IPROTO_ERROR_CODE) {
> if (mp_typeof(**pos) != MP_UINT)
> return -1;
> code = mp_decode_uint(pos);
> + if (code > UINT32_MAX)
> + return -1;
> } else if (key == IPROTO_ERROR_MESSAGE) {
> if (mp_typeof(**pos) != MP_STR)
> return -1;
Applied.
> diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
> index ae45f18b0..718ef15a2 100644
> --- a/test/unit/xrow.cc
> +++ b/test/unit/xrow.cc
> @@ -32,6 +32,7 @@ extern "C" {
> #include "unit.h"
> } /* extern "C" */
> #include "trivia/util.h"
> +#include "box/error.h"
> #include "box/xrow.h"
> #include "box/iproto_constants.h"
> #include "uuid/tt_uuid.h"
> @@ -255,7 +256,7 @@ error_stack_entry_encode(char *pos, const char *err_str)
> void
> test_xrow_error_stack_decode()
> {
> - plan(17);
> + plan(24);
> char buffer[2048];
> /*
> * To start with, let's test the simplest and obsolete
> @@ -392,9 +393,51 @@ test_xrow_error_stack_decode()
> xrow_decode_error(&header);
> last = diag_last_error(diag_get());
> isnt(last, NULL, "xrow_decode succeed: diag has been set");
> + is(box_error_code(last), 0, "xrow_decode last error code is default 0");
> is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode corrupted stack: "
> "stack's map wrong key");
>
> + /* Overflow error code. */
> + 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, IPROTO_ERROR_CODE);
> + pos = mp_encode_uint(pos, (uint64_t)1 << 40);
> + 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(box_error_code(last), 159, "xrow_decode failed, took code from "
> + "header");
> + is(strcmp(last->errmsg, ""), 0, "xrow_decode failed, message is not "
> + "decoded");
> +
> + /* Overflow error key. */
> + 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, 0xff00000000 | 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(box_error_code(last), 0, "xrow_decode last error code is default 0");
> + is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode message is fine "
> + "even without the code");
It is likely to be the same as "Bad key in the packet.". At least it
seems to cover the same execution path.
Squashed both patches on the top of branch.
More information about the Tarantool-patches
mailing list