From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 5B6B44696C3 for ; Fri, 3 Apr 2020 01:20:34 +0300 (MSK) 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> From: Vladislav Shpilevoy Message-ID: Date: Fri, 3 Apr 2020 00:20:32 +0200 MIME-Version: 1.0 In-Reply-To: <20200402140100.GA30923@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org 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. >>> 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. ==================== 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; 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"); + check_plan(); } diff --git a/test/unit/xrow.result b/test/unit/xrow.result index d24e9ea4f..7213ab6c7 100644 --- a/test/unit/xrow.result +++ b/test/unit/xrow.result @@ -53,7 +53,7 @@ ok 1 - subtests ok 9 - decoded sync ok 10 - decoded bodycnt ok 2 - subtests - 1..17 + 1..24 ok 1 - xrow_decode succeed: diag has been set ok 2 - xrow_decode succeed: error is parsed ok 3 - xrow_decode succeed: diag has been set @@ -70,7 +70,14 @@ ok 2 - subtests ok 14 - xrow_decode succeed: diag has been set ok 15 - xrow_decode corrupted stack: stack's map wrong value type ok 16 - xrow_decode succeed: diag has been set - ok 17 - xrow_decode corrupted stack: stack's map wrong key + ok 17 - xrow_decode last error code is default 0 + ok 18 - xrow_decode corrupted stack: stack's map wrong key + ok 19 - xrow_decode succeed: diag has been set + ok 20 - xrow_decode failed, took code from header + ok 21 - xrow_decode failed, message is not decoded + ok 22 - xrow_decode succeed: diag has been set + ok 23 - xrow_decode last error code is default 0 + ok 24 - xrow_decode message is fine even without the code ok 3 - subtests 1..1 ok 1 - request_str