Bug 93908

Summary: _dbus_marshall_validate_test skips half the tests
Product: dbus Reporter: Nick Lewycky <nlewycky>
Component: coreAssignee: 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

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.