Re: [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

On 28/11/2019 13:28, Chris Sosnin wrote:
> Hi! Thank you for your suggestions.
> I am sorry for being impatient.
> 
>> 1. To not duplicate code, you can add a label, and make a
>> goto to there from 'default'. Or vice versa - go to default
>> from there.
> 
> I labeled the first return as error:
> 
> +error:
> +		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num,
> +			 tt_sprintf("\"%.*s\"", len, opcode));
> +		return NULL;
> +	}
> ...
> default:
> -		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
> -		return NULL;
> +		goto error;
> }
> 
>> 2. I would better assign it after xrow_update_op_by()
>> returned not NULL. Because MessagePack strings are not
>> zero terminated. So 'opcode' after mp_decode_str() may
>> actually point at invalid memory instead of zero
>> terminator in case of an empty string.
> 
> In this case we would still return with an error, however
> I changed it to be your way:

It will return an error, yes. In case the dereference of
the invalid pointer won't crash by luck.

> 
> +	const char *opcode = mp_decode_str(expr, &len);
> +	op->meta = xrow_update_op_by(opcode, len, op_num);
>  	if (op->meta == NULL)
>  		return -1;
> +	op->opcode = *opcode;
> 
> diff --git a/src/box/xrow_update.c b/src/box/xrow_update.c
> index 123db081a..db215aada 100644
> --- a/src/box/xrow_update.c
> +++ b/src/box/xrow_update.c
> @@ -620,12 +625,16 @@ xrow_update_op_decode(struct xrow_update_op *op, int index_base,
>  			 "update operation name must be a string");
>  		return -1;
>  	}
> -	op->opcode = *mp_decode_str(expr, &len);
> -	op->meta = xrow_update_op_by(op->opcode);
> +	const char *opcode = mp_decode_str(expr, &len);
> +	op->meta = xrow_update_op_by(opcode, len, op_num);
>  	if (op->meta == NULL)
>  		return -1;
> +	op->opcode = *opcode;
>  	if (arg_count != op->meta->arg_count) {
> -		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
> +		const char *str = tt_sprintf("wrong number of arguments, "\
> +		                 "expected %u, got %u",
> +		                 op->meta->arg_count, arg_count);

Seems like the comment 3 from the previous review is not
fixed. The indentation is still incorrect here.

> +		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num, str);
>  		return -1;
>  	}
>  	int32_t field_no = 0;