Bug 66300 - [PATCH] several small fixes for DBusString, DBusError, DBusList and README.win
Summary: [PATCH] several small fixes for DBusString, DBusError, DBusList and README.win
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-28 06:51 UTC by Chengwei Yang
Modified: 2013-06-28 12:18 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/4] Fix two small typos in README.win (841 bytes, patch)
2013-06-28 06:52 UTC, Chengwei Yang
Details | Splinter Review
[PATCH 2/4] Remove invoke of va_end before va_start (1.08 KB, patch)
2013-06-28 06:53 UTC, Chengwei Yang
Details | Splinter Review
[PATCH 3/4] Release list lock a little early (700 bytes, patch)
2013-06-28 06:53 UTC, Chengwei Yang
Details | Splinter Review
[PATCH 4/4] DBusString: fix a typo and check byte value strictly (1.14 KB, patch)
2013-06-28 06:53 UTC, Chengwei Yang
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chengwei Yang 2013-06-28 06:51:30 UTC

    
Comment 1 Chengwei Yang 2013-06-28 06:52:27 UTC
Created attachment 81612 [details] [review]
[PATCH 1/4] Fix two small typos in README.win
Comment 2 Chengwei Yang 2013-06-28 06:53:05 UTC
Created attachment 81613 [details] [review]
[PATCH 2/4] Remove invoke of va_end before va_start
Comment 3 Chengwei Yang 2013-06-28 06:53:26 UTC
Created attachment 81615 [details] [review]
[PATCH 3/4] Release list lock a little early
Comment 4 Chengwei Yang 2013-06-28 06:53:50 UTC
Created attachment 81616 [details] [review]
[PATCH 4/4] DBusString: fix a typo and check byte value strictly
Comment 5 Simon McVittie 2013-06-28 10:19:14 UTC
Comment on attachment 81612 [details] [review]
[PATCH 1/4] Fix two small typos in README.win

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

OK
Comment 6 Simon McVittie 2013-06-28 10:20:43 UTC
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 7 Simon McVittie 2013-06-28 10:22:58 UTC
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 8 Simon McVittie 2013-06-28 10:34:04 UTC
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 9 Simon McVittie 2013-06-28 10:39:30 UTC
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 10 Simon McVittie 2013-06-28 11:03:56 UTC
Comment on attachment 81612 [details] [review]
[PATCH 1/4] Fix two small typos in README.win

applied, 1.7.6
Comment 11 Simon McVittie 2013-06-28 11:04:06 UTC
Comment on attachment 81613 [details] [review]
[PATCH 2/4] Remove invoke of va_end before va_start

applied, 1.6.14/1.7.6
Comment 12 Simon McVittie 2013-06-28 11:10:29 UTC
(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.
Comment 13 Chengwei Yang 2013-06-28 11:46:24 UTC
(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.
Comment 14 Chengwei Yang 2013-06-28 11:48:37 UTC
(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.
Comment 15 Simon McVittie 2013-06-28 12:18:20 UTC
(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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.