Bug 93205 - Misuse of Enum in bus_config_load()
Summary: Misuse of Enum in bus_config_load()
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: x86-64 (AMD64) Linux (All)
: low trivial
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-12-02 07:52 UTC by Deepika Aggarwal
Modified: 2016-07-01 15:48 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
patch file for misuse enum (575 bytes, patch)
2015-12-02 07:52 UTC, Deepika Aggarwal
Details | Splinter Review
Revised patch for misuse enum comparison fix (1019 bytes, patch)
2015-12-03 06:19 UTC, Deepika Aggarwal
Details | Splinter Review
Revised patch full testsuite run log files (7.21 KB, text/plain)
2015-12-03 06:20 UTC, Deepika Aggarwal
Details

Description Deepika Aggarwal 2015-12-02 07:52:58 UTC
Created attachment 120262 [details] [review]
patch file for misuse enum

Misuse of Enum in bus_config_load()

In bus_config_load, an enum-typed expression is used in a Boolean conditional context. 
This contribution fixes enum being treated as bool in below code statement by checking it with appropiate enum data type:

if (!XML_Parse (expat, data_str, _dbus_string_get_length (&data), TRUE))

The test suite works fine after applying the proposed patch. 
Summary is as below:

============================================================================
Testsuite summary for dbus 1.10.99
============================================================================
# TOTAL: 18
# PASS:  18
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================
Comment 1 Simon McVittie 2015-12-02 11:02:44 UTC
(In reply to Deepika Aggarwal from comment #0)
> In bus_config_load, an enum-typed expression is used in a Boolean
> conditional context. 

Is this invalid C? I would expect use of an enum-typed expression in boolean context to be the same as use of a plain integer in boolean context, i.e. it is true if its numeric value is nonzero. XML_STATUS_ERROR is zero.

The members of XML_Status appear to be XML_STATUS_ERROR = 0, XML_STATUS_OK = 1 and XML_STATUS_SUSPENDED = 2. We never actually suspend parsing, so in practice we'll only get ERROR or OK.

It seems XML_Parse() originally returned int, with values 0 or 1, and the XML_Status enum was added later: https://mail.python.org/pipermail/expat-discuss/2002-August/000602.html
Comment 2 Simon McVittie 2015-12-02 11:07:16 UTC
Comment on attachment 120262 [details] [review]
patch file for misuse enum

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

I don't object to being more explicit about the comparison; the feature of returning an XML_Status instead of a plain integer seems to be old enough that it shouldn't cause compatibility issues in practice.

This patch seems wrong, though.

If you revise the patch please attach it in "git format-patch" format, so that it will be correctly attributed to you. With a plain diff I have to guess what name and email address (if any) you want to end up in the project's git history.

::: dbus/bus/config-loader-expat.c
@@ +242,4 @@
>  
>      data_str = _dbus_string_get_const_data (&data);
>  
> +    if (XML_Parse (expat, data_str, _dbus_string_get_length (&data), TRUE) != XML_STATUS_ERROR)

I'm surprised this passes tests, because its sense is the reverse of what it should be. It used to be

if (!XML_Parse (...))

which is equivalent to

if (XML_Parse (...) == 0)

but you've changed it into

if (XML_Parse (...) != 0)

Please check that you are running the full test suite (./configure --enable-tests - you will need GLib and Python, and the resulting binaries are not recommended for production use).
Comment 3 Deepika Aggarwal 2015-12-03 06:19:23 UTC
Created attachment 120293 [details] [review]
Revised patch for misuse enum comparison fix
Comment 4 Deepika Aggarwal 2015-12-03 06:20:59 UTC
Created attachment 120294 [details]
Revised patch full testsuite run log files

Testuite run log file
Comment 5 Deepika Aggarwal 2015-12-03 06:25:21 UTC
Pl. find attached the correct updated and formatted patch.
(0001-Misuse-of-Enum-in-bus_config_load-during-comparison.patch)

The previous patch got uploaded by mistake.

Its better to check specifically with enum status XML_STATUS_ERROR though the enum value is 0 only.
It will avoid any chance of mistake in future and better readability as the enum has other non zero values also. 

I ran the full test suite with this patch (including Glib and Python)
Below are the test results:
============================================================================
Testsuite summary for dbus 1.10.99
============================================================================
# TOTAL: 99
# PASS:  99
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

Also, attached are the full test suite run log file.
(dbustestsuite_misuseEnum.log)
Comment 6 Simon McVittie 2016-07-01 15:48:40 UTC
Fixed in git for 1.11.4, thanks.

This doesn't seem to be a user-observable bug, so I'm not backporting it to the 1.10.x stable branch.


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.