Bug 93909

Summary:	fix error handling for invalid auth state in auth script
Product:	dbus	Reporter:	Nick Lewycky <nlewycky>
Component:	core	Assignee:	D-Bus Maintainers <dbus>
Status:	RESOLVED FIXED	QA Contact:	D-Bus Maintainers <dbus>
Severity:	normal
Priority:	medium	Keywords:	patch
Version:	git master
Hardware:	Other
OS:	All
Whiteboard:	review+
i915 platform:		i915 features:
Attachments:	proposed fix proposed fix #2

Description Nick Lewycky 2016-01-28 21:05:48 UTC

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 1 Simon McVittie 2016-02-02 13:56:32 UTC

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.

Comment 2 Simon McVittie 2016-02-02 13:56:59 UTC

Other than that this looks fine.

Comment 3 Nick Lewycky 2016-02-03 00:30:18 UTC

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.

Comment 4 Simon McVittie 2016-07-01 15:41:17 UTC

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.