Bug 93909 - fix error handling for invalid auth state in auth script
Summary: fix error handling for invalid auth state in auth script
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-01-28 21:05 UTC by Nick Lewycky
Modified: 2016-07-01 15:41 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
proposed fix (1.92 KB, patch)
2016-01-28 21:05 UTC, Nick Lewycky
Details | Splinter Review
proposed fix #2 (1.92 KB, patch)
2016-02-03 00:30 UTC, Nick Lewycky
Details | Splinter Review

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.