Created attachment 121364 [details] [review] proposed fix In dbus-auth-script.c _dbus_auth_script_run, there is unreachable code intended for error handling: expected = auth_state_from_string (&line); if (expected < 0) { _dbus_warn ("bad auth state given to EXPECT_STATE\n"); goto parse_failed; } The type of 'expected' is enum DBusAuthState which converts to an unsigned int (on Linux x86-64), and therefore can't be less than 0. I've attached a patch which adds a new enumerator constant DBUS_AUTH_STATE_INVALID = -1, which causes the enum type to convert to some size of signed int (an enum must be compatible with char or some integer type that includes all the enum's members, C99 6.7.2.2/4). I've gone through the rest of the uses of this type and found only one that needed updating. No test changes. Spotted by clang -Wtautological-compare.
Comment on attachment 121364 [details] [review] proposed fix Review of attachment 121364 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-auth.h @@ +38,5 @@ > DBUS_AUTH_STATE_WAITING_FOR_MEMORY, > DBUS_AUTH_STATE_HAVE_BYTES_TO_SEND, > DBUS_AUTH_STATE_NEED_DISCONNECT, > + DBUS_AUTH_STATE_AUTHENTICATED, > + DBUS_AUTH_STATE_INVALID = -1, Surely this should go at the top of the enum? I'd love to be able to use a trailing comma like this (it reduces the diffstat and hence conflicts when adding lines to an enum)... but strictly speaking it isn't valid ISO C89, and some old compiler (I forget which) actually diagnoses this as an error, so please don't.
Other than that this looks fine.
Created attachment 121478 [details] [review] proposed fix #2 The order of the enums doesn't matter, since it increases each time you don't specify a number and starts at 0 by default. I think it's common to put invalid last, but I'm happy to move it wherever you like? You're absolutely right about the trailing comma, I've removed that.
I don't think this actually matters in practice (no user-observable bug), because this is in test code anyway; so I'm not applying it to a stable-branch. Fixed in git for 1.11.4, thanks.
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.