Bug 66300

Created attachment 81612 [details] [review]
[PATCH 1/4] Fix two small typos in README.win

Created attachment 81613 [details] [review]
[PATCH 2/4] Remove invoke of va_end before va_start

Created attachment 81615 [details] [review]
[PATCH 3/4] Release list lock a little early

Created attachment 81616 [details] [review]
[PATCH 4/4] DBusString: fix a typo and check byte value strictly

Comment on attachment 81612 [details] [review]
[PATCH 1/4] Fix two small typos in README.win

Review of attachment 81612 [details] [review]:
-----------------------------------------------------------------

OK

Comment on attachment 81613 [details] [review]
[PATCH 2/4] Remove invoke of va_end before va_start

Review of attachment 81613 [details] [review]:
-----------------------------------------------------------------

Good catch.

Comment on attachment 81615 [details] [review]
[PATCH 3/4] Release list lock a little early

Review of attachment 81615 [details] [review]:
-----------------------------------------------------------------

Why? I can see that holding the lock at that point is not *strictly* necessary, but I don't think it's a bug to be holding the lock either? As far as I can see, the best this can possibly achieve is a tiny performance/parallelization increase.

I'm reluctant to apply any changes of the form "do less locking" without a very good justification.

Comment on attachment 81616 [details] [review]
[PATCH 4/4] DBusString: fix a typo and check byte value strictly

Review of attachment 81616 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-string.c
@@ +977,4 @@
>  dbus_bool_t
>  _dbus_string_insert_2_aligned (DBusString         *str,
>                                 int                 insert_at,
> +                               const unsigned char octets[2])

OK, but this shouldn't have been the same patch as the other change - they're completely orthogonal (and the other one is incorrect).

@@ +2236,5 @@
>    };
>  
> +  if (byte < 0 || byte > 0xff)
> +    return FALSE;
> +

" * @returns #FALSE if no memory"

This is not "no memory": if anything, this is a programming error (which would mean _dbus_assert, rather than returning).

However, looking at the callers of this function, on platforms where char is signed, _dbus_address_append_escaped() will pass a value between -128 and +127 to this function. So, if this was an assertion failure, I suspect it might actually be possible to trigger it.

I think the right solution here would be to change the type of the parameter 'byte' to unsigned char, and change _dbus_address_append_escaped() so p is of type "unsigned char *" (which would mean using a cast when initially assigning to p).

I think this also deserves a regression test, preferably calling dbus_address_escape_value() on a parameter containing bytes > 127, in a test-case similar to test/syntax.c. (For instance, you could check that dbus_address_escape_value ("\xab") is "%ab".)

Comment on attachment 81613 [details] [review]
[PATCH 2/4] Remove invoke of va_end before va_start

Review of attachment 81613 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-errors.c
@@ +235,4 @@
>   * must ensure the name and message are global data that won't be
>   * freed. You probably want dbus_set_error() instead, in most cases.
>   * 
> + * @param error the error or #NULL

This cosmetic fix shouldn't really have been the same commit as the va_end(), which is a genuine bugfix. Please don't combine unrelated changes just because they're nearby.

Comment on attachment 81612 [details] [review]
[PATCH 1/4] Fix two small typos in README.win

applied, 1.7.6

Comment on attachment 81613 [details] [review]
[PATCH 2/4] Remove invoke of va_end before va_start

applied, 1.6.14/1.7.6

(In reply to comment #8)
> ::: dbus/dbus-string.c
> @@ +977,4 @@
> >  dbus_bool_t
> >  _dbus_string_insert_2_aligned (DBusString         *str,
> >                                 int                 insert_at,
> > +                               const unsigned char octets[2])
> 
> OK, but this shouldn't have been the same patch as the other change -
> they're completely orthogonal (and the other one is incorrect).

I applied this bit for 1.7.6.

(In reply to comment #7)
> Comment on attachment 81615 [details] [review] [review]
> [PATCH 3/4] Release list lock a little early
> 
> Review of attachment 81615 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Why? I can see that holding the lock at that point is not *strictly*
> necessary, but I don't think it's a bug to be holding the lock either? As
> far as I can see, the best this can possibly achieve is a tiny
> performance/parallelization increase.
> 
> I'm reluctant to apply any changes of the form "do less locking" without a
> very good justification.

OK, if I can found any more in later work, a good performance test may help.

(In reply to comment #8)
> Comment on attachment 81616 [details] [review] [review]
> [PATCH 4/4] DBusString: fix a typo and check byte value strictly
> 
> Review of attachment 81616 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-string.c
> @@ +977,4 @@
> >  dbus_bool_t
> >  _dbus_string_insert_2_aligned (DBusString         *str,
> >                                 int                 insert_at,
> > +                               const unsigned char octets[2])
> 
> OK, but this shouldn't have been the same patch as the other change -
> they're completely orthogonal (and the other one is incorrect).
> 
> @@ +2236,5 @@
> >    };
> >  
> > +  if (byte < 0 || byte > 0xff)
> > +    return FALSE;
> > +
> 
> " * @returns #FALSE if no memory"
> 
> This is not "no memory": if anything, this is a programming error (which
> would mean _dbus_assert, rather than returning).
> 
> However, looking at the callers of this function, on platforms where char is
> signed, _dbus_address_append_escaped() will pass a value between -128 and
> +127 to this function. So, if this was an assertion failure, I suspect it
> might actually be possible to trigger it.
> 
> I think the right solution here would be to change the type of the parameter
> 'byte' to unsigned char, and change _dbus_address_append_escaped() so p is
> of type "unsigned char *" (which would mean using a cast when initially
> assigning to p).
> 
> I think this also deserves a regression test, preferably calling
> dbus_address_escape_value() on a parameter containing bytes > 127, in a
> test-case similar to test/syntax.c. (For instance, you could check that
> dbus_address_escape_value ("\xab") is "%ab".)

mark it as TODO.

(In reply to comment #8)
> However, looking at the callers of this function, on platforms where char is
> signed, _dbus_address_append_escaped() will pass a value between -128 and
> +127 to this function. So, if this was an assertion failure, I suspect it
> might actually be possible to trigger it.

This is Bug #53499, in fact.

Closing this bug, since the rest is fixed.