Bug 30350

Created attachment 38916 [details] [review]
Implement dbus_message_iter_get_n_elements

Included in the patch is a simple test case and the documentation of dbus_message_iter_get_array_len now refers to the new function.

I agree, but I suggest calling this dbus_message_iter_get_element_count instead. "get_n_elements" sounds like you pass an n and you get that many elements.

Created attachment 38917 [details] [review]
Implement dbus_message_iter_get_element_count

No strong opinion personally. Updated patch with the new name.

Review of attachment 38917 [details] [review]:

::: dbus/dbus-message-util.c
@@ +1415,3 @@
+  dbus_message_iter_init (message, &iter);
+  _dbus_assert (dbus_message_iter_get_element_count (&iter) == 3);
+  dbus_message_unref (message);

Should probably test a couple of fixed-size types also, such as byte and int32.

actually using append_basic you can likely write a generic test that runs on any basic type and then call it with different typecodes.

Testing an array of structs or array of arrays would not be bad either.

::: dbus/dbus-message.c
@@ +2180,3 @@
+ * Returns the number of elements in the array-typed value pointed
+ * to by the iterator.
+ *

I think it would be good to note here that it's O(n) and therefore kind of a bad idea to use.

Well, really it should not be O(n) for fixed-size-element arrays... I think if we're going to have this it should probably use the array len (see deprecated function below) and divide by sizeof(element).
There may be a get-size-of-fixed-type utility function floating in the code somewhere, I don't remember.

Created attachment 39098 [details] [review]
Implement dbus_message_iter_get_element_count #2

(In reply to comment #4)
> Should probably test a couple of fixed-size types also, such as byte and int32.
> Testing an array of structs or array of arrays would not be bad either.

I made it loop through the basic types now, and also test an array of structures.

> I think it would be good to note here that it's O(n) and therefore kind of a
> bad idea to use.
> Well, really it should not be O(n) for fixed-size-element arrays

I added a special-case for fixed-size arrays and documented it accordingly.

Review of attachment 39098 [details] [review]:

looks good, some nitpicks.

::: dbus/dbus-message-util.c
@@ +1418,3 @@
+      dbus_message_iter_open_container (&iter, DBUS_TYPE_ARRAY,
+                                        basic_types + i, &array_iter);
+      for (some = 0; some < 3; some++)

this for() could use braces around the body

@@ +1427,3 @@
+      _dbus_assert (dbus_message_iter_get_element_count (&iter) == some);
+      dbus_message_unref (message);
+      basic_types[i] = '\0';

I think some compilers or some gcc settings get upset about assigning to string literals.

::: dbus/dbus-message.c
@@ +2180,3 @@
+ * Returns the number of elements in the array-typed value pointed
+ * to by the iterator.
+ * Note that this function is O(1) for fixed-size arrays but O(n)

"arrays of fixed-size types" might be clearer

@@ +2181,3 @@
+ * to by the iterator.
+ * Note that this function is O(1) for fixed-size arrays but O(n)
+ * otherwise, so it may be a bad idea to use it.

"O(n) for arrays of variable-length types such as strings"

@@ +2189,3 @@
+{
+  DBusMessageRealIter *real = (DBusMessageRealIter *)iter;
+  int atype = _dbus_type_reader_get_current_type(&real->u.reader);

this assignment should be dropped down below the return_val_if_fail that checks validity of 'real'

@@ +2205,3 @@
+      return total_len / alignment;
+    }
+

clearer to then do "else" here I think with the non-fixed case

Created attachment 39150 [details] [review]
Implement dbus_message_iter_get_element_count #3

(In reply to comment #6)
> Review of attachment 39098 [details] [review]:

I used an allocated signature string in the unit tests now and avoided the int atype declaration. I also incorporated the stylistic and rethorical suggestions.

Review of attachment 39150 [details] [review]:

This looks ready to commit to me, thanks

::: dbus/dbus-message.c
@@ +2187,3 @@
+ * @returns the number of elements in the array
+ */
+int dbus_message_iter_get_element_count (DBusMessageIter* iter)

should linebreak after "int" here I missed on first time through the patch.
Maybe someone can just fix that when committing though.

Review of attachment 39150 [details] [review]:

This looks ready to commit to me, thanks

::: dbus/dbus-message.c
@@ +2187,3 @@
+ * @returns the number of elements in the array
+ */
+int dbus_message_iter_get_element_count (DBusMessageIter* iter)

should linebreak after "int" here I missed on first time through the patch.
Maybe someone can just fix that when committing though.

Created attachment 84731 [details] [review]
Implement dbus_message_iter_get_element_count

From: Christian Dywan

According unit tests are added to _dbus_message_test.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=30350
Reviewed-by: Havoc Pennington <hp@pobox.com>

Created attachment 84732 [details] [review]
Fix whitespace as per Havoc's review (in 2010)

---

Not going to bother with review for this one :-P

Created attachment 84733 [details] [review]
Avoid reading beyond the length of a variable

Appending &some as DBUS_TYPE_INT64, DBUS_TYPE_UINT64 or DBUS_TYPE_DOUBLE,
where "some" is an int, reads beyond the bounds of that variable.
Use a zero-filled DBusBasicValue instead.

(In reply to comment #12)
> Avoid reading beyond the length of a variable

I'm OK with applying Christian's patch as long as we also apply this.

Created attachment 84738 [details] [review]
Avoid reading beyond the length of a variable

Appending &some as DBUS_TYPE_INT64, DBUS_TYPE_UINT64 or DBUS_TYPE_DOUBLE,
where "some" is an int, reads beyond the bounds of that variable.
Use a zero-filled DBusBasicValue instead.

---

v2: sorry, forgot to --amend.

Comment on attachment 38917 [details] [review]
Implement dbus_message_iter_get_element_count

>From b9c996ca75d3f7434449cfa60629e3a285910f1b Mon Sep 17 00:00:00 2001
>From: Christian Dywan <christian.dywan@lanedo.com>
>Date: Thu, 23 Sep 2010 19:22:53 +0200
>Subject: [PATCH] Implement dbus_message_iter_get_element_count
>
>See https://bugs.freedesktop.org/show_bug.cgi?id=30350
>---
> dbus/dbus-message-util.c |   20 ++++++++++++++++++++
> dbus/dbus-message.c      |   34 +++++++++++++++++++++++++++-------
> dbus/dbus-message.h      |    3 +++
> 3 files changed, 50 insertions(+), 7 deletions(-)
>
>diff --git a/dbus/dbus-message-util.c b/dbus/dbus-message-util.c
>index f972c8a..9a502f7 100644
>--- a/dbus/dbus-message-util.c
>+++ b/dbus/dbus-message-util.c
>@@ -1396,6 +1396,26 @@ _dbus_message_test (const char *test_data_dir)
>   check_memleaks ();
>   _dbus_check_fdleaks();
> 
>+  /* Test enumeration of array elements */
>+  message = dbus_message_new_method_call ("de.ende.test",
>+                                          "/de/ende/test",
>+                                          "de.ende.Test",
>+                                          "ArtistName");
>+  _dbus_assert (message != NULL);
>+  dbus_message_iter_init_append (message, &iter);
>+  dbus_message_iter_open_container (&iter, DBUS_TYPE_ARRAY,
>+                                    DBUS_TYPE_STRING_AS_STRING, &array_iter);
>+  s = "SomeThingToSay";
>+  dbus_message_iter_append_basic (&array_iter, DBUS_TYPE_STRING, &s);
>+  s = "MoreHumbugToCome";
>+  dbus_message_iter_append_basic (&array_iter, DBUS_TYPE_STRING, &s);
>+  s = "WerewolvesMinotaursKirins";
>+  dbus_message_iter_append_basic (&array_iter, DBUS_TYPE_STRING, &s);
>+  dbus_message_iter_close_container (&iter, &array_iter);
>+  dbus_message_iter_init (message, &iter);
>+  _dbus_assert (dbus_message_iter_get_element_count (&iter) == 3);
>+  dbus_message_unref (message);
>+
>   /* Check that we can abandon a container */
>   message = dbus_message_new_method_call ("org.freedesktop.DBus.TestService",
> 		  			  "/org/freedesktop/TestPath",
>diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
>index b19697e..bd11e18 100644
>--- a/dbus/dbus-message.c
>+++ b/dbus/dbus-message.c
>@@ -2177,20 +2177,40 @@ dbus_message_iter_get_basic (DBusMessageIter  *iter,
> }
> 
> /**
>+ * Returns the number of elements in the array-typed value pointed
>+ * to by the iterator.
>+ *
>+ * @param iter the iterator
>+ * @returns the number of elements in the array
>+ */
>+int dbus_message_iter_get_element_count (DBusMessageIter* iter)
>+{
>+  DBusMessageRealIter *real = (DBusMessageRealIter *)iter;
>+  DBusTypeReader array;
>+  int n_elements = 0;
>+
>+  _dbus_return_val_if_fail (_dbus_message_iter_check (real), 0);
>+  _dbus_return_val_if_fail (_dbus_type_reader_get_current_type (&real->u.reader)
>+                            == DBUS_TYPE_ARRAY, 0);
>+
>+  _dbus_type_reader_recurse (&real->u.reader, &array);
>+  while (_dbus_type_reader_get_current_type (&array) != DBUS_TYPE_INVALID)
>+    {
>+      ++n_elements;
>+      _dbus_type_reader_next (&array);
>+    }
>+  return n_elements;
>+}
>+
>+/**
>  * Returns the number of bytes in the array as marshaled in the wire
>  * protocol. The iterator must currently be inside an array-typed
>  * value.
>  *
>  * This function is deprecated on the grounds that it is stupid.  Why
>  * would you want to know how many bytes are in the array as marshaled
>- * in the wire protocol?  For now, use the n_elements returned from
>- * dbus_message_iter_get_fixed_array() instead, or iterate over the
>- * array values and count them.
>+ * in the wire protocol?  Use dbus_message_iter_get_element_count() instead.
>  *
>- * @todo introduce a variant of this get_n_elements that returns
>- * the number of elements, though with a non-fixed array it will not
>- * be very efficient, so maybe it's not good.
>- * 
>  * @param iter the iterator
>  * @returns the number of bytes in the array
>  */
>diff --git a/dbus/dbus-message.h b/dbus/dbus-message.h
>index 5500492..fa1b2d4 100644
>--- a/dbus/dbus-message.h
>+++ b/dbus/dbus-message.h
>@@ -226,6 +226,9 @@ void        dbus_message_iter_recurse          (DBusMessageIter *iter,
> DBUS_EXPORT
> void        dbus_message_iter_get_basic        (DBusMessageIter *iter,
>                                                 void            *value);
>+DBUS_EXPORT
>+int         dbus_message_iter_get_element_count(DBusMessageIter *iter);
>+
> #ifndef DBUS_DISABLE_DEPRECATED
> /* This function returns the wire protocol size of the array in bytes,
>  * you do not want to know that probably
>-- 
>1.7.2.3
>

(In reply to Simon McVittie from comment #13)
> I'm OK with applying Christian's patch as long as we also apply [...]

(In reply to Simon McVittie from comment #14)
> Avoid reading beyond the length of a variable

This has been waiting for review nearly 2 years, as a fix for a nearly 5 year old patch. That's quite long enough; I'm just going to apply it.

Fixed in git for 1.9.16.