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.
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.