Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area

[Nikita Pettik <korablev@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.