Re: [PATCH v2 04/11] dbus: Fix parsing GVariant header/body information

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Andrew,

On 04/11/2016 08:56 PM, Andrew Zaborowski wrote:
> In GVariant messages there's no body size field (it is reserved and must
> be sent as 0) or header size information or signature.  We do still add
> the signature as a field same as in Dbus-1 because it's not prohibited
> but we can't rely on it being there.  The message needs to be parsed as
> a single GVariant blob if we want to split it into a header and a body.
> ---
>   ell/dbus-kernel.c  |   3 --
>   ell/dbus-message.c | 108 ++++++++++++++++++++++++++++++++++++-----------------
>   2 files changed, 73 insertions(+), 38 deletions(-)
>

<snip>

> @@ -653,6 +651,7 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
>   {
>   	const struct dbus_header *hdr = data;
>   	struct l_dbus_message *message;
> +	size_t body_pos;
>
>   	if (unlikely(size < DBUS_HEADER_SIZE))
>   		return NULL;
> @@ -661,28 +660,62 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
>
>   	message->refcount = 1;
>
> -	message->header_size = align_len(DBUS_HEADER_SIZE +
> -						hdr->field_length, 8);
> -	message->body_size = hdr->body_length;
> +	if (hdr->version == 1) {
> +		message->header_size = align_len(DBUS_HEADER_SIZE +
> +							hdr->field_length, 8);
> +		message->body_size = hdr->body_length;
>
> -	if (message->header_size + message->body_size < size) {
> -		l_free(message);
> -		return NULL;
> +		if (message->header_size + message->body_size < size)
> +			goto free;
> +
> +		body_pos = message->header_size;
> +	} else {
> +		struct l_dbus_message_iter iter, header,
> +			variant, structure, body;

Please break this up into multiple struct declarations.

e.g.

struct l_dbus_message_iter iter;
struct l_dbus_message_iter header, variant, body;

Just to group them better and make this more readable.

> +
> +		if (!_gvariant_iter_init(&iter, message, "((yyyyuta{tv})v)",
> +						NULL, data, size))

So quoting or pointing to:
https://wiki.gnome.org/Projects/GLib/GDBus/Version2
might be in order here.

It says: "The canonical type of a D-Bus version two message is 
(yyyyuta{tv}v)."

Also, you might want to put in a quick blurb why we're not using the 
canonical structure.

> +			goto free;
> +
> +		if (!_gvariant_iter_enter_struct(&iter, &structure))
> +			goto free;

Since the outer structure is implicit, do we really need this part? 
Can't we simply omit the outer '()'s? If so, the code can be simplified.

> +
> +		if (!_gvariant_iter_enter_struct(&structure, &header))
> +			goto free;
> +
> +		if (!_gvariant_iter_enter_variant(&structure, &variant))
> +			goto free;
> +
> +		if (!_gvariant_iter_enter_struct(&variant, &body))
> +			goto free;
> +
> +		message->header_size = align_len(header.len - header.pos, 8);

why do we need an align_len here?  This was probably specific to the 
classic and old kdbus formats.

> +		message->body_size = body.len - body.pos;
> +		message->signature = l_strndup(body.sig_start + body.sig_pos,
> +						body.sig_len - body.sig_pos);
> +		message->signature_free = true;
> +		message->header_end = header.len;
> +		body_pos = body.data + body.pos - data;
>   	}
>
>   	message->header = l_malloc(message->header_size);
>   	message->body = l_malloc(message->body_size);
>
>   	memcpy(message->header, data, message->header_size);
> -	memcpy(message->body, data + message->header_size, message->body_size);
> +	memcpy(message->body, data + body_pos, message->body_size);
>
>   	message->sealed = true;
>
>   	/* If the field is absent message->signature will remain NULL */
> -	get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE, 'g',
> -						&message->signature);
> +	if (hdr->version == 1)
> +		get_header_field(message, DBUS_MESSAGE_FIELD_SIGNATURE,
> +					'g', &message->signature);
>
>   	return message;
> +
> +free:
> +	l_free(message);
> +	return NULL;
>   }
>
>   struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
> @@ -698,6 +731,13 @@ struct l_dbus_message *dbus_message_build(void *header, size_t header_size,
>   	if (unlikely(!valid_header(hdr)))
>   		return NULL;
>
> +	/*
> +	 * With GVariant we need to know the signature, use
> +	 * dbus_message_from_blob instead.
> +	 */
> +	if (unlikely(hdr->version != 1))
> +		return NULL;
> +
>   	message = l_new(struct l_dbus_message, 1);
>
>   	message->refcount = 1;
> @@ -827,21 +867,18 @@ static void build_header(struct l_dbus_message *message, const char *signature)
>   	char *generated_signature;
>   	struct dbus_header *hdr;
>   	size_t header_size;
> +	bool gvariant;
>
> -	if (_dbus_message_is_gvariant(message))
> +	gvariant = _dbus_message_is_gvariant(message);
> +
> +	if (gvariant)
>   		driver = &gvariant_driver;
>   	else
>   		driver = &dbus1_driver;
>
>   	builder = driver->new(message->header, message->header_size);
>
> -	if (_dbus_message_is_gvariant(message)) {
> -		uint32_t field_length = 0;
> -		driver->append_basic(builder, 'u', &field_length);
> -	}
> -
> -	driver->enter_array(builder, _dbus_message_is_gvariant(message) ?
> -				"(tv)" : "(yv)");
> +	driver->enter_array(builder, gvariant ? "(tv)" : "(yv)");
>
>   	if (message->path) {
>   		add_field(builder, driver, DBUS_MESSAGE_FIELD_PATH,
> @@ -906,15 +943,16 @@ static void build_header(struct l_dbus_message *message, const char *signature)
>   	hdr = message->header;
>
>   	if (_dbus_message_is_gvariant(message))
> -		hdr->field_length = header_size - 16;
> -
> -	hdr->body_length = message->body_size;
> +		hdr->body_length = 0;
> +	else
> +		hdr->body_length = message->body_size;
>
>   	/* We must align the end of the header to an 8-byte boundary */
>   	message->header_size = align_len(header_size, 8);
>   	message->header = l_realloc(message->header, message->header_size);
>   	memset(message->header + header_size, 0,
>   			message->header_size - header_size);
> +	message->header_end = header_size;
>   }
>
>   struct container {
>

Regards,
-Denis