Bug 93908 - _dbus_marshall_validate_test skips half the tests
Summary: _dbus_marshall_validate_test skips half the tests
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
Whiteboard: review?
Keywords: patch
Depends on:
Reported: 2016-01-28 20:33 UTC by Nick Lewycky
Modified: 2016-02-08 20:09 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

proposed fix (1.32 KB, patch)
2016-01-28 20:33 UTC, Nick Lewycky
Details | Splinter Review

Description Nick Lewycky 2016-01-28 20:33:24 UTC
Created attachment 121363 [details] [review]
proposed fix

Due to a duplicate 'i++' in run_validity_tests in dbus-marshall-validate-util.c, only the even tests are run.

With this fixed, we discover that the test for "a)" fails because it produces DBUS_INVALID_MISSING_ARRAY_ELEMENT_TYPE instead of DBUS_INVALID_STRUCT_ENDED_BUT_NOT_STARTED.

The attached patch fixes the bug and disables the failing test.

<advertisement>Found by clang -Wfor-loop-analysis.</advertisement>  ;)
Comment 1 Simon McVittie 2016-02-08 17:42:46 UTC
Comment on attachment 121363 [details] [review]
proposed fix

Review of attachment 121363 [details] [review]:

::: dbus/dbus-marshal-validate-util.c
@@ -61,5 @@
>                        v, tests[i].data);
>            _dbus_assert_not_reached ("test failed");
>          }
> -
> -      ++i;

Thanks, well caught.

@@ +81,4 @@

This is not correct; commenting-out code is usually bad.

If a test has accidentally not been run, and its result differs from what the code says it should be, the question to answer is: is the actual result acceptable? If yes, we should assert that the result is that (or perhaps that it is one of the reasonable options); if no, we should fix whatever is being tested.

In this case, MISSING_ARRAY_ELEMENT_TYPE seems just as reasonable for "a)" as STRUCT_ENDED_BUT_NOT_STARTED - this string has both of those problems, it's just a matter of which one we hit first, and the author of this test happens to have picked the one not matching the implementation. So I'm going to uncomment this line and assert the real result.
Comment 2 Simon McVittie 2016-02-08 20:09:38 UTC
Fixed in git for 1.10.8 and 1.11.2, 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.