[PATCH] dbus: More complete buffer size check in dbus_message_from_blob

[PATCH] dbus: More complete buffer size check in dbus_message_from_blob

[Andrew Zaborowski]

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

---
 ell/dbus-message.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 84d42d4..f9e13e2 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -643,9 +643,14 @@ struct l_dbus_message *dbus_message_from_blob(const void *data, size_t size)
 
 	message->header_size = align_len(DBUS_HEADER_SIZE +
 						hdr->field_length, 8);
-	message->header = l_malloc(message->header_size);
-
 	message->body_size = hdr->body_length;
+
+	if (message->header_size + message->body_size < size) {
+		l_free(message);
+		return NULL;
+	}
+
+	message->header = l_malloc(message->header_size);
 	message->body = l_malloc(message->body_size);
 
 	memcpy(message->header, data, message->header_size);
-- 
2.5.0

[PATCH] dbus: Replace copy_params/size_params with macros.

[Andrew Zaborowski]

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

Passing va_list to functions this way doesn't work on i386 gcc, va_list
seems to be a simple pointer as recommended by stdarg(3).  Passing a
pointer to va_list seems to fix this, but stdarg(3) also says:

   If ap is passed to a function that uses va_arg(ap,type), then the value
   of ap is undefined after the return of that function.

so it's better to just use a macro.
---
 ell/dbus-service.c | 98 ++++++++++++++++++++++--------------------------------
 1 file changed, 40 insertions(+), 58 deletions(-)

diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index c1ed9ec..e11a6d0 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -259,39 +259,34 @@ void _dbus_interface_introspection(struct l_dbus_interface *interface,
 	l_string_append(buf, "\t</interface>\n");
 }
 
-static char *copy_params(char *dest, const char *signature, va_list args)
-{
-	const char *pname;
-	const char *sig;
-
-	for (sig = signature; *sig; sig++) {
-		sig = _dbus_signature_end(sig);
-		if (!sig)
-			return NULL;
-
-		pname = va_arg(args, const char *);
-		dest = stpcpy(dest, pname) + 1;
-	}
-
-	return dest;
-}
-
-static bool size_params(const char *signature, va_list args, unsigned int *len)
-{
-	const char *pname;
-	const char *sig;
-
-	for (sig = signature; *sig; sig++) {
-		sig = _dbus_signature_end(sig);
-		if (!sig)
-			return false;
-
-		pname = va_arg(args, const char *);
-		*len += strlen(pname) + 1;
-	}
-
-	return true;
-}
+#define COPY_PARAMS(dest, signature, args)	\
+	do {	\
+		const char *pname;	\
+		const char *sig;	\
+		dest = stpcpy(dest, signature) + 1;	\
+		for (sig = signature; *sig; sig++) {	\
+			sig = _dbus_signature_end(sig);	\
+			pname = va_arg(args, const char *);	\
+			dest = stpcpy(dest, pname) + 1;	\
+		}	\
+	} while(0)
+
+#define SIZE_PARAMS(signature, args)	\
+	({	\
+		unsigned int len = strlen(signature) + 1;	\
+		const char *pname;	\
+		const char *sig;	\
+		for (sig = signature; *sig; sig++) {	\
+			sig = _dbus_signature_end(sig);	\
+			if (!sig) {	\
+				len = 0;	\
+				break;	\
+			}	\
+			pname = va_arg(args, const char *);	\
+			len += strlen(pname) + 1;	\
+		}	\
+		len;	\
+	})
 
 LIB_EXPORT bool l_dbus_interface_method(struct l_dbus_interface *interface,
 					const char *name, uint32_t flags,
@@ -318,19 +313,16 @@ LIB_EXPORT bool l_dbus_interface_method(struct l_dbus_interface *interface,
 		return false;
 
 	/* Pre-calculate the needed meta-info length */
-	return_info_len = strlen(return_sig) + 1;
-	param_info_len = strlen(param_sig) + 1;
-
 	va_start(args, param_sig);
 
-	if (!size_params(return_sig, args, &return_info_len))
-		goto error;
-
-	if (!size_params(param_sig, args, &param_info_len))
-		goto error;
+	return_info_len = SIZE_PARAMS(return_sig, args);
+	param_info_len = SIZE_PARAMS(param_sig, args);
 
 	va_end(args);
 
+	if (!return_info_len || !param_info_len)
+		return false;
+
 	info = l_malloc(sizeof(*info) + return_info_len +
 					param_info_len + strlen(name) + 1);
 	info->cb = cb;
@@ -345,22 +337,16 @@ LIB_EXPORT bool l_dbus_interface_method(struct l_dbus_interface *interface,
 	 * lookups during the message dispatch procedures.
 	 */
 	p = info->metainfo + info->name_len + param_info_len + 1;
-	p = stpcpy(p, return_sig) + 1;
-	p = copy_params(p, return_sig, args);
+	COPY_PARAMS(p, return_sig, args);
 
 	p = info->metainfo + info->name_len + 1;
-	p = stpcpy(p, param_sig) + 1;
-	p = copy_params(p, param_sig, args);
+	COPY_PARAMS(p, param_sig, args);
 
 	va_end(args);
 
 	l_queue_push_tail(interface->methods, info);
 
 	return true;
-
-error:
-	va_end(args);
-	return false;
 }
 
 LIB_EXPORT bool l_dbus_interface_signal(struct l_dbus_interface *interface,
@@ -382,17 +368,14 @@ LIB_EXPORT bool l_dbus_interface_signal(struct l_dbus_interface *interface,
 		return false;
 
 	/* Pre-calculate the needed meta-info length */
-	metainfo_len = strlen(name) + 1;
-	metainfo_len += strlen(signature) + 1;
-
 	va_start(args, signature);
+	metainfo_len = SIZE_PARAMS(signature, args);
+	va_end(args);
 
-	if (!size_params(signature, args, &metainfo_len)) {
-		va_end(args);
+	if (!metainfo_len)
 		return false;
-	}
 
-	va_end(args);
+	metainfo_len += strlen(name) + 1;
 
 	info = l_malloc(sizeof(*info) + metainfo_len);
 	info->flags = flags;
@@ -401,8 +384,7 @@ LIB_EXPORT bool l_dbus_interface_signal(struct l_dbus_interface *interface,
 	p = stpcpy(info->metainfo, name) + 1;
 
 	va_start(args, signature);
-	p = stpcpy(p, signature) + 1;
-	p = copy_params(p, signature, args);
+	COPY_PARAMS(p, signature, args);
 	va_end(args);
 
 	l_queue_push_tail(interface->signals, info);
-- 
2.5.0

Re: [PATCH] dbus: Replace copy_params/size_params with macros.

[Denis Kenzior]

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

Hi Andrew,

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> Passing va_list to functions this way doesn't work on i386 gcc, va_list
> seems to be a simple pointer as recommended by stdarg(3).  Passing a
> pointer to va_list seems to fix this, but stdarg(3) also says:
>
>     If ap is passed to a function that uses va_arg(ap,type), then the value
>     of ap is undefined after the return of that function.
>
> so it's better to just use a macro.
> ---
>   ell/dbus-service.c | 98 ++++++++++++++++++++++--------------------------------
>   1 file changed, 40 insertions(+), 58 deletions(-)

Makes sense to me.  Thanks for looking into this one.

Applied, thanks.

Regards,
-Denis

[PATCH] unit: Add int64_t casts in dbus tests failing on i386.

[Andrew Zaborowski]

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

Make sure the values are passed as 64-bit ones, adding LL would probably
also work for gcc.
---
 unit/test-dbus-message.c     | 2 +-
 unit/test-gvariant-message.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/unit/test-dbus-message.c b/unit/test-dbus-message.c
index 4008d2b..ce58769 100644
--- a/unit/test-dbus-message.c
+++ b/unit/test-dbus-message.c
@@ -1555,7 +1555,7 @@ static void build_basic_7(const void *data)
 {
 	struct l_dbus_message *msg = build_message(data);
 
-	l_dbus_message_set_arguments(msg, "t", 10000);
+	l_dbus_message_set_arguments(msg, "t", (uint64_t) 10000);
 
 	compare_message(msg, data);
 }
diff --git a/unit/test-gvariant-message.c b/unit/test-gvariant-message.c
index 324c341..5573e63 100644
--- a/unit/test-gvariant-message.c
+++ b/unit/test-gvariant-message.c
@@ -208,7 +208,8 @@ static void build_basic_1(const void *data)
 
 	result = l_dbus_message_set_arguments(msg, "bynqiuxtd", true, 255,
 						-32, 32, -24, 24,
-						140179142606749, 99L, 5.0);
+						(uint64_t) 140179142606749,
+						(int64_t) 99, 5.0);
 	assert(result);
 
 	_dbus_message_set_serial(msg, 1111);
-- 
2.5.0

Re: [PATCH] unit: Add int64_t casts in dbus tests failing on i386.

[Denis Kenzior]

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

Hi Andrew,

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> Make sure the values are passed as 64-bit ones, adding LL would probably
> also work for gcc.
> ---
>   unit/test-dbus-message.c     | 2 +-
>   unit/test-gvariant-message.c | 3 ++-
>   2 files changed, 3 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH] dbus: Take iter->sig_len into account in l_dbus_message_iter_get_variant

[Andrew Zaborowski]

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

---
 ell/dbus-message.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index f9e13e2..30c09b9 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -1379,7 +1379,8 @@ LIB_EXPORT bool l_dbus_message_iter_get_variant(
 	if (unlikely(!iter))
 		return false;
 
-	if (!iter->sig_start || strcmp(iter->sig_start, signature))
+	if (!iter->sig_start || strlen(signature) != iter->sig_len ||
+			memcmp(iter->sig_start, signature, iter->sig_len))
 		return false;
 
 	va_start(args, signature);
-- 
2.5.0

Re: [PATCH] dbus: Take iter->sig_len into account in l_dbus_message_iter_get_variant

[Denis Kenzior]

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

Hi Andrew,

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-message.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH] gvariant: Exclude container's offsets from child iterator len.

[Andrew Zaborowski]

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

---
 ell/gvariant-util.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 1aaddc3..61e5b52 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -497,6 +497,7 @@ static const void *next_item(struct l_dbus_message_iter *iter,
 	bool last_member;
 	unsigned int sig_len;
 	unsigned int offset_len;
+	unsigned int len = iter->len;
 
 	memcpy(sig, iter->sig_start + iter->sig_pos,
 			iter->sig_len - iter->sig_pos);
@@ -529,7 +530,14 @@ static const void *next_item(struct l_dbus_message_iter *iter,
 	}
 
 	if (iter->container_type != DBUS_CONTAINER_TYPE_ARRAY && last_member) {
-		*out_item_size = iter->len - iter->pos;
+		offset_len = offset_length(iter->len, 0);
+		len = iter->len;
+
+		if (iter->offsets && iter->offsets + offset_len <
+				iter->data + len)
+			len = iter->offsets + offset_len - iter->data;
+
+		*out_item_size = len - iter->pos;
 		goto done;
 	}
 
-- 
2.5.0

Re: [PATCH] gvariant: Exclude container's offsets from child iterator len.

[Denis Kenzior]

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

Hi Andrew,

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> ---
>   ell/gvariant-util.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
> index 1aaddc3..61e5b52 100644
> --- a/ell/gvariant-util.c
> +++ b/ell/gvariant-util.c
> @@ -497,6 +497,7 @@ static const void *next_item(struct l_dbus_message_iter *iter,
>   	bool last_member;
>   	unsigned int sig_len;
>   	unsigned int offset_len;
> +	unsigned int len = iter->len;

Looks like this belongs in the if block below.

>
>   	memcpy(sig, iter->sig_start + iter->sig_pos,
>   			iter->sig_len - iter->sig_pos);
> @@ -529,7 +530,14 @@ static const void *next_item(struct l_dbus_message_iter *iter,
>   	}
>
>   	if (iter->container_type != DBUS_CONTAINER_TYPE_ARRAY && last_member) {
> -		*out_item_size = iter->len - iter->pos;
> +		offset_len = offset_length(iter->len, 0);
> +		len = iter->len;
> +
> +		if (iter->offsets && iter->offsets + offset_len <
> +				iter->data + len)
> +			len = iter->offsets + offset_len - iter->data;
> +
> +		*out_item_size = len - iter->pos;

This looks fine to me.  I'm guessing the location of the child 
iterator's offsets was being messed up?  Hence variable length field 
sizes were incorrect.  Right?

>   		goto done;
>   	}
>
>

Regards,
-Denis

Re: [PATCH] gvariant: Exclude container's offsets from child iterator len.

[Andrzej Zaborowski]

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

Hi Denis,

On 21 March 2016 at 18:57, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
>>
>> ---
>>   ell/gvariant-util.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
>> index 1aaddc3..61e5b52 100644
>> --- a/ell/gvariant-util.c
>> +++ b/ell/gvariant-util.c
>> @@ -497,6 +497,7 @@ static const void *next_item(struct
>> l_dbus_message_iter *iter,
>>         bool last_member;
>>         unsigned int sig_len;
>>         unsigned int offset_len;
>> +       unsigned int len = iter->len;
>
>
> Looks like this belongs in the if block below.

True, it's also redundant.

>
>>
>>         memcpy(sig, iter->sig_start + iter->sig_pos,
>>                         iter->sig_len - iter->sig_pos);
>> @@ -529,7 +530,14 @@ static const void *next_item(struct
>> l_dbus_message_iter *iter,
>>         }
>>
>>         if (iter->container_type != DBUS_CONTAINER_TYPE_ARRAY &&
>> last_member) {
>> -               *out_item_size = iter->len - iter->pos;
>> +               offset_len = offset_length(iter->len, 0);
>> +               len = iter->len;
>> +
>> +               if (iter->offsets && iter->offsets + offset_len <
>> +                               iter->data + len)
>> +                       len = iter->offsets + offset_len - iter->data;
>> +
>> +               *out_item_size = len - iter->pos;
>
>
> This looks fine to me.  I'm guessing the location of the child iterator's
> offsets was being messed up?  Hence variable length field sizes were
> incorrect.  Right?

I think that is what was happening, in the end it would read some
child value from a completely wrong place.

Best regards

[PATCH] gvariant: Reset container's variable_is_last for fixed-size structs.

[Andrew Zaborowski]

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

We reset container->variable_is_last after basic fixed-size types but
not after structs and dicts.  This would probably produce incorrect
results for fixed-size structs that are the last element in a
variable-size struct, e.g. (s(u))
---
 ell/gvariant-util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 61e5b52..4601222 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -994,6 +994,8 @@ static bool leave_struct_dict_common(struct dbus_builder *builder,
 	if (_gvariant_is_fixed_size(container->signature)) {
 		int alignment = _gvariant_get_alignment(container->signature);
 		grow_body(builder, 0, alignment);
+
+		parent->variable_is_last = false;
 	} else {
 		size_t offset;
 
-- 
2.5.0

Re: [PATCH] gvariant: Reset container's variable_is_last for fixed-size structs.

[Denis Kenzior]

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

Hi Andrew,

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> We reset container->variable_is_last after basic fixed-size types but
> not after structs and dicts.  This would probably produce incorrect
> results for fixed-size structs that are the last element in a
> variable-size struct, e.g. (s(u))
> ---
>   ell/gvariant-util.c | 2 ++
>   1 file changed, 2 insertions(+)

I think you are correct on this one.  Can we add a quickie test for this?

I went ahead and applied the patch.

Regards,
-Denis

[PATCH] dbus: Fix returned body_size in _dbus1_builder_finish.

[Andrew Zaborowski]

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

Seems we missed this in the _mark and _rewind patch.
---
 ell/dbus-util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/dbus-util.c b/ell/dbus-util.c
index 9d5b5af..ab0600a 100644
--- a/ell/dbus-util.c
+++ b/ell/dbus-util.c
@@ -1275,7 +1275,7 @@ char *_dbus1_builder_finish(struct dbus_builder *builder,
 	builder->signature = NULL;
 
 	*body = builder->body;
-	*body_size = builder->body_size;
+	*body_size = builder->body_pos;
 	builder->body = NULL;
 	builder->body_size = 0;
 
-- 
2.5.0

Re: [PATCH] dbus: Fix returned body_size in _dbus1_builder_finish.

[Denis Kenzior]

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

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> Seems we missed this in the _mark and _rewind patch.

Yep!

> ---
>   ell/dbus-util.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH 1/2] dbus: Add _gvariant_builder_mark and _rewind.

[Andrew Zaborowski]

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

---
 ell/dbus-message.c     |  2 ++
 ell/gvariant-private.h |  2 ++
 ell/gvariant-util.c    | 78 +++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/ell/dbus-message.c b/ell/dbus-message.c
index 30c09b9..ef6a50d 100644
--- a/ell/dbus-message.c
+++ b/ell/dbus-message.c
@@ -773,6 +773,8 @@ static struct builder_driver gvariant_driver = {
 	.enter_array = _gvariant_builder_enter_array,
 	.leave_array = _gvariant_builder_leave_array,
 	.finish = _gvariant_builder_finish,
+	.mark = _gvariant_builder_mark,
+	.rewind = _gvariant_builder_rewind,
 	.new = _gvariant_builder_new,
 	.free = _gvariant_builder_free,
 };
diff --git a/ell/gvariant-private.h b/ell/gvariant-private.h
index 2a20feb..f8f1313 100644
--- a/ell/gvariant-private.h
+++ b/ell/gvariant-private.h
@@ -47,6 +47,8 @@ struct dbus_builder *_gvariant_builder_new(void *body, size_t body_size);
 void _gvariant_builder_free(struct dbus_builder *builder);
 bool _gvariant_builder_append_basic(struct dbus_builder *builder,
 					char type, const void *value);
+bool _gvariant_builder_mark(struct dbus_builder *builder);
+bool _gvariant_builder_rewind(struct dbus_builder *builder);
 char *_gvariant_builder_finish(struct dbus_builder *builder,
 				void **body, size_t *body_size);
 bool _gvariant_builder_enter_struct(struct dbus_builder *builder,
diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 4601222..79f1a68 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -753,7 +753,15 @@ struct dbus_builder {
 	struct l_string *signature;
 	void *body;
 	size_t body_size;
+	size_t body_pos;
 	struct l_queue *containers;
+	struct {
+		struct container *container;
+		int sig_end;
+		size_t body_pos;
+		size_t offset_index;
+		bool variable_is_last : 1;
+	} mark;
 };
 
 struct container {
@@ -770,18 +778,19 @@ struct container {
 static inline size_t grow_body(struct dbus_builder *builder,
 					size_t len, unsigned int alignment)
 {
-	size_t size = align_len(builder->body_size, alignment);
+	size_t size = align_len(builder->body_pos, alignment);
 
 	if (size + len > builder->body_size) {
 		builder->body = l_realloc(builder->body, size + len);
-
-		if (size - builder->body_size > 0)
-			memset(builder->body + builder->body_size, 0,
-						size - builder->body_size);
-
 		builder->body_size = size + len;
 	}
 
+	if (size - builder->body_pos > 0)
+		memset(builder->body + builder->body_pos, 0,
+			size - builder->body_pos);
+
+	builder->body_pos = size + len;
+
 	return size;
 }
 
@@ -840,7 +849,7 @@ static void container_append_struct_offsets(struct container *container,
 	if (container->offset_index == 0)
 		return;
 
-	offset_size = offset_length(builder->body_size,
+	offset_size = offset_length(builder->body_pos,
 						container->offset_index);
 	i = container->offset_index - 1;
 
@@ -863,7 +872,7 @@ static void container_append_array_offsets(struct container *container,
 	if (container->offset_index == 0)
 		return;
 
-	offset_size = offset_length(builder->body_size,
+	offset_size = offset_length(builder->body_pos,
 						container->offset_index);
 	start = grow_body(builder, offset_size * container->offset_index, 1);
 
@@ -888,6 +897,9 @@ struct dbus_builder *_gvariant_builder_new(void *body, size_t body_size)
 
 	builder->body = body;
 	builder->body_size = body_size;
+	builder->body_pos = body_size;
+
+	builder->mark.container = root;
 
 	return builder;
 }
@@ -1003,7 +1015,7 @@ static bool leave_struct_dict_common(struct dbus_builder *builder,
 			return false;
 
 		container_append_struct_offsets(container, builder);
-		offset = builder->body_size - parent->start;
+		offset = builder->body_pos - parent->start;
 		parent->offsets[parent->offset_index++] = offset;
 		parent->variable_is_last = true;
 	}
@@ -1086,7 +1098,7 @@ bool _gvariant_builder_leave_variant(struct dbus_builder *builder)
 	if (!grow_offsets(parent))
 		return false;
 
-	offset = builder->body_size - parent->start;
+	offset = builder->body_pos - parent->start;
 	parent->offsets[parent->offset_index++] = offset;
 	parent->variable_is_last = true;
 
@@ -1166,7 +1178,7 @@ bool _gvariant_builder_leave_array(struct dbus_builder *builder)
 	if (!grow_offsets(parent))
 		return false;
 
-	offset = builder->body_size - parent->start;
+	offset = builder->body_pos - parent->start;
 	parent->offsets[parent->offset_index++] = offset;
 	parent->variable_is_last = true;
 
@@ -1225,7 +1237,7 @@ bool _gvariant_builder_append_basic(struct dbus_builder *builder,
 	start = grow_body(builder, len, alignment);
 	memcpy(builder->body + start, value, len);
 
-	offset = builder->body_size - container->start;
+	offset = builder->body_pos - container->start;
 	container->offsets[container->offset_index++] = offset;
 	container->variable_is_last = true;
 
@@ -1235,6 +1247,46 @@ bool _gvariant_builder_append_basic(struct dbus_builder *builder,
 	return true;
 }
 
+bool _gvariant_builder_mark(struct dbus_builder *builder)
+{
+	struct container *container = l_queue_peek_head(builder->containers);
+
+	builder->mark.container = container;
+
+	if (l_queue_length(builder->containers) == 1)
+		builder->mark.sig_end = l_string_length(builder->signature);
+	else
+		builder->mark.sig_end = container->sigindex;
+
+	builder->mark.body_pos = builder->body_pos;
+	builder->mark.offset_index = container->offset_index;
+	builder->mark.variable_is_last = container->variable_is_last;
+
+	return true;
+}
+
+bool _gvariant_builder_rewind(struct dbus_builder *builder)
+{
+	struct container *container;
+
+	while ((container = l_queue_peek_head(builder->containers)) !=
+				builder->mark.container) {
+		container_free(container);
+		l_queue_pop_head(builder->containers);
+	}
+
+	builder->body_pos = builder->mark.body_pos;
+	container->offset_index = builder->mark.offset_index;
+	container->variable_is_last = builder->mark.variable_is_last;
+
+	if (l_queue_length(builder->containers) == 1)
+		l_string_truncate(builder->signature, builder->mark.sig_end);
+	else
+		container->sigindex = builder->mark.sig_end;
+
+	return true;
+}
+
 char *_gvariant_builder_finish(struct dbus_builder *builder,
 				void **body, size_t *body_size)
 {
@@ -1259,7 +1311,7 @@ char *_gvariant_builder_finish(struct dbus_builder *builder,
 		container_append_struct_offsets(root, builder);
 
 	*body = builder->body;
-	*body_size = builder->body_size;
+	*body_size = builder->body_pos;
 	builder->body = NULL;
 	builder->body_size = 0;
 
-- 
2.5.0

Re: [PATCH 1/2] dbus: Add _gvariant_builder_mark and _rewind.

[Denis Kenzior]

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

Hi Andrew,

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-message.c     |  2 ++
>   ell/gvariant-private.h |  2 ++
>   ell/gvariant-util.c    | 78 +++++++++++++++++++++++++++++++++++++++++---------
>   3 files changed, 69 insertions(+), 13 deletions(-)

Applied, thanks.

Regards,
-Denis

[PATCH 2/2] unit: reuse mark+rewind, complex 1 tests for gvariant-message.

[Andrew Zaborowski]

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

---
 unit/test-gvariant-message.c | 158 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 152 insertions(+), 6 deletions(-)

diff --git a/unit/test-gvariant-message.c b/unit/test-gvariant-message.c
index 5573e63..c2cb811 100644
--- a/unit/test-gvariant-message.c
+++ b/unit/test-gvariant-message.c
@@ -79,6 +79,40 @@ static const struct message_data message_data_basic_1 = {
 	.binary_len	= 184,
 };
 
+static const unsigned char message_binary_complex_1[] = {
+	0x6c, 0x01, 0x00, 0x02, 0x4f, 0x00, 0x00, 0x00, 0x57, 0x04, 0x00, 0x00,
+	0x86, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x2f, 0x63, 0x6f, 0x6d, 0x2f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65,
+	0x2f, 0x6f, 0x62, 0x6a, 0x65, 0x63, 0x74, 0x00, 0x00, 0x6f, 0x00, 0x00,
+	0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x6d, 0x65, 0x74, 0x68,
+	0x6f, 0x64, 0x00, 0x00, 0x73, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x63, 0x6f, 0x6d, 0x2e,
+	0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x69, 0x6e, 0x74, 0x65,
+	0x72, 0x66, 0x61, 0x63, 0x65, 0x00, 0x00, 0x73, 0x06, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x63, 0x6f, 0x6d, 0x2e, 0x65, 0x78, 0x61, 0x6d,
+	0x70, 0x6c, 0x65, 0x00, 0x00, 0x73, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x6f, 0x61, 0x7b, 0x73, 0x76, 0x7d, 0x00, 0x00,
+	0x67, 0x1e, 0x31, 0x58, 0x6e, 0x81, 0x00, 0x00, 0x2f, 0x63, 0x6f, 0x6d,
+	0x2f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2f, 0x6f, 0x62, 0x6a,
+	0x65, 0x63, 0x74, 0x00, 0x00, 0x00, 0x00, 0x00, 0x4e, 0x61, 0x6d, 0x65,
+	0x00, 0x00, 0x00, 0x00, 0x4c, 0x69, 0x6e, 0x75, 0x73, 0x20, 0x54, 0x6f,
+	0x72, 0x76, 0x61, 0x6c, 0x64, 0x73, 0x00, 0x00, 0x73, 0x05, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x44, 0x65, 0x76, 0x65, 0x6c, 0x6f, 0x70, 0x65,
+	0x72, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x62, 0x0a,
+	0x1a, 0x34, 0x14,
+};
+
+static const struct message_data message_data_complex_1 = {
+	.type		= "method_call",
+	.path		= "/com/example/object",
+	.interface	= "com.example.interface",
+	.member		= "method",
+	.destination	= "com.example",
+	.signature	= "oa{sv}",
+	.binary		= message_binary_complex_1,
+	.binary_len	= sizeof(message_binary_complex_1),
+};
+
 static struct l_dbus_message *check_message(const struct message_data *msg_data)
 {
 	struct l_dbus_message *msg;
@@ -133,6 +167,17 @@ static struct l_dbus_message *check_message(const struct message_data *msg_data)
 	return msg;
 }
 
+static struct l_dbus_message *build_message(const struct message_data *msg_data)
+{
+	struct l_dbus_message *msg;
+
+	msg = _dbus_message_new_method_call(2, msg_data->destination,
+			msg_data->path, msg_data->interface, msg_data->member);
+	assert(msg);
+
+	return msg;
+}
+
 static void compare_message(struct l_dbus_message *msg,
 					const struct message_data *msg_data)
 {
@@ -198,14 +243,9 @@ static void parse_basic_1(const void *data)
 
 static void build_basic_1(const void *data)
 {
-	const struct message_data *msg_data = data;
-	struct l_dbus_message *msg;
+	struct l_dbus_message *msg = build_message(data);
 	bool result;
 
-	msg = _dbus_message_new_method_call(2, msg_data->destination,
-			msg_data->path, msg_data->interface, msg_data->member);
-	assert(msg);
-
 	result = l_dbus_message_set_arguments(msg, "bynqiuxtd", true, 255,
 						-32, 32, -24, 24,
 						(uint64_t) 140179142606749,
@@ -217,6 +257,104 @@ static void build_basic_1(const void *data)
 	compare_message(msg, data);
 }
 
+static void check_complex_1(const void *data)
+{
+	struct l_dbus_message *msg = check_message(data);
+	struct l_dbus_message_iter dict, iter;
+	const char *path, *str;
+	bool result, val;
+
+	result = l_dbus_message_get_arguments(msg, "oa{sv}", &path, &dict);
+	assert(result);
+	assert(!strcmp(path, "/com/example/object"));
+
+	result = l_dbus_message_iter_next_entry(&dict, &str, &iter);
+	assert(result);
+	assert(!strcmp(str, "Name"));
+
+	result = l_dbus_message_iter_get_variant(&iter, "s", &str);
+	assert(result);
+	assert(!strcmp(str, "Linus Torvalds"));
+
+	result = l_dbus_message_iter_next_entry(&dict, &str, &iter);
+	assert(result);
+	assert(!strcmp(str, "Developer"));
+
+	result = l_dbus_message_iter_get_variant(&iter, "b", &val);
+	assert(result);
+	assert(val);
+
+	result = l_dbus_message_iter_next_entry(&dict, &str, &iter);
+	assert(!result);
+
+	l_dbus_message_unref(msg);
+}
+
+static void build_complex_1(const void *data)
+{
+	struct l_dbus_message *msg = build_message(data);
+	bool result;
+
+	result = l_dbus_message_set_arguments(msg, "oa{sv}",
+						"/com/example/object", 2,
+						"Name", "s", "Linus Torvalds",
+						"Developer", "b", true);
+	assert(result);
+
+	_dbus_message_set_serial(msg, 1111);
+
+	compare_message(msg, data);
+}
+
+static void builder_rewind(const void *data)
+{
+	struct l_dbus_message *msg = build_message(data);
+	struct l_dbus_message_builder *builder;
+	bool b = true;
+
+	builder = l_dbus_message_builder_new(msg);
+	assert(builder);
+
+	assert(l_dbus_message_builder_append_basic(builder, 'o',
+							"/com/example/object"));
+
+	assert(l_dbus_message_builder_enter_array(builder, "{sv}"));
+
+	assert(_dbus_message_builder_mark(builder));
+
+	assert(l_dbus_message_builder_enter_dict(builder, "sv"));
+	assert(l_dbus_message_builder_append_basic(builder, 's', "Name"));
+	assert(l_dbus_message_builder_enter_variant(builder, "s"));
+	assert(l_dbus_message_builder_append_basic(builder, 's',
+							"Invalid"));
+	assert(l_dbus_message_builder_leave_variant(builder));
+	assert(l_dbus_message_builder_leave_dict(builder));
+
+	assert(_dbus_message_builder_rewind(builder));
+
+	assert(l_dbus_message_builder_enter_dict(builder, "sv"));
+	assert(l_dbus_message_builder_append_basic(builder, 's', "Name"));
+	assert(l_dbus_message_builder_enter_variant(builder, "s"));
+	assert(l_dbus_message_builder_append_basic(builder, 's',
+							"Linus Torvalds"));
+	assert(l_dbus_message_builder_leave_variant(builder));
+	assert(l_dbus_message_builder_leave_dict(builder));
+
+	assert(l_dbus_message_builder_enter_dict(builder, "sv"));
+	assert(l_dbus_message_builder_append_basic(builder, 's', "Developer"));
+	assert(l_dbus_message_builder_enter_variant(builder, "b"));
+	assert(l_dbus_message_builder_append_basic(builder, 'b', &b));
+	assert(l_dbus_message_builder_leave_variant(builder));
+	assert(l_dbus_message_builder_leave_dict(builder));
+
+	assert(l_dbus_message_builder_leave_array(builder));
+
+	assert(l_dbus_message_builder_finalize(builder));
+	l_dbus_message_builder_destroy(builder);
+
+	compare_message(msg, data);
+}
+
 int main(int argc, char *argv[])
 {
 	l_test_init(&argc, &argv);
@@ -224,5 +362,13 @@ int main(int argc, char *argv[])
 	l_test_add("Basic 1 (parse)", parse_basic_1, &message_data_basic_1);
 	l_test_add("Basic 1 (build)", build_basic_1, &message_data_basic_1);
 
+	l_test_add("Complex 1 (parse)", check_complex_1,
+						&message_data_complex_1);
+	l_test_add("Complex 1 (build)", build_complex_1,
+						&message_data_complex_1);
+
+	l_test_add("Message Builder Rewind Complex 1", builder_rewind,
+						&message_data_complex_1);
+
 	return l_test_run();
 }
-- 
2.5.0

Re: [PATCH 2/2] unit: reuse mark+rewind, complex 1 tests for gvariant-message.

[Denis Kenzior]

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

Hi Andrew,

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> ---
>   unit/test-gvariant-message.c | 158 +++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 152 insertions(+), 6 deletions(-)
>

Applied.  Need you to resubmit the iter offsets patch for this to pass 
properly.

Regards,
-Denis

Re: [PATCH] dbus: More complete buffer size check in dbus_message_from_blob

[Denis Kenzior]

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

Hi Andrew,

On 03/19/2016 01:00 AM, Andrew Zaborowski wrote:
> ---
>   ell/dbus-message.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis