From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 3781B4696C3 for ; Fri, 3 Apr 2020 05:16:48 +0300 (MSK) Date: Fri, 3 Apr 2020 02:16:47 +0000 From: Nikita Pettik Message-ID: <20200403021647.GD946@tarantool.org> References: <3bb7c695b34356137897b953281543c46fe3bafa.1585097339.git.korablev@tarantool.org> <20200401162618.GD26447@tarantool.org> <20200401222456.GA27659@tarantool.org> <64be3c70-4ec9-0743-0b6d-e7453239db4a@tarantool.org> <20200402140100.GA30923@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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 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.