Re: [PATCH v3 4/9] gvariant: Fix empty structure/array parsing and error check

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Hi Andrew,

On 04/14/2016 07:33 PM, Andrew Zaborowski wrote:
> Allow empty signatures in _gvariant_num_children and check the return
> value for errors or we might crash.
> ---
>   ell/gvariant-util.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
> index e07bd2f..791fc41 100644
> --- a/ell/gvariant-util.c
> +++ b/ell/gvariant-util.c
> @@ -201,14 +201,14 @@ int _gvariant_num_children(const char *sig)
>   	if (strlen(sig) > 255)
>   		return false;
>
> -	do {
> +	while (*s) {
>   		s = validate_next_type(s, &a);
>
>   		if (!s)
>   			return -1;
>
>   		num_children += 1;
> -	} while (*s);
> +	}
>

So I have a nagging bad feeling about this one.  The code of this 
function was copied from _gvariant_valid_signature.  So changing this 
strongly implies changing _gvariant_valid_signature as well...

Passing in "" won't crash this function, so where do we crash?

>   	return num_children;
>   }
> @@ -374,7 +374,7 @@ static bool gvariant_iter_init_internal(struct l_dbus_message_iter *iter,
>   		unsigned int alignment : 4;
>   		size_t end;		/* Index past the end of the type */
>   	} *children;
> -	uint8_t n_children;
> +	int n_children;
>
>   	if (sig_end) {
>   		size_t len = sig_end - sig_start;
> @@ -392,6 +392,9 @@ static bool gvariant_iter_init_internal(struct l_dbus_message_iter *iter,
>   	iter->pos = 0;
>
>   	n_children = _gvariant_num_children(subsig);
> +	if (n_children < 0)
> +		return false;
> +
>   	children = l_new(struct gvariant_type_info, n_children);

So why would we be allocating a zero-sized array?  Do we need to special 
case this somehow?

Can I get some unit test data to play with?

>
>   	for (p = sig_start, i = 0; i < n_children; i++) {
>

Regards,
-Denis