S2259 C++ False negative with Boost.Test terminating assertions

Rules and Languages Report False-positive / False-negative...
1

Hello,
This was tested on SonarQube 9.8 Developer Edition.

The Boost.Test framework has a number of assertions that can abort execution. The code below shouldn’t be flagged, because t->foo() will not be reached.

#define BOOST_TEST_MODULE example
#include <boost/test/included/unit_test.hpp>

struct Test { void foo() {} };

BOOST_AUTO_TEST_CASE(require)
{
	Test* t;
	BOOST_TEST_REQUIRE(t, "no t");
	t->foo();
}

BOOST_AUTO_TEST_CASE(fail)
{
	Test* t;
	if(!t) BOOST_FAIL("no t");
	t->foo();
}

Is this something that the S2259 rule can handle?

Thanks for your help

4

At glance, I think probably the code expanded from the BOOST_FAIL() and BOOST_TEST_REQUIRE() macros refer to functions or constructs that are not decorated with the noreturn attribute, hence we must assume that they return, and we cannot rule out that execution path.

To confirm this theory, I would suggest to generate a reproducer for your case.

To generate the reproducer file:

  • Search in the analysis log for the full path of the source file for which you want to create a reproducer (for instance, a file that contains a false-positive). You will have to use exactly this name (same case, / or \…)
  • Add the reproducer option to the scanner configuration:
    sonar.cfamily.reproducer=“Full path to the .cpp”
  • Re-run the scanner to generate a file named sonar-cfamily.reproducer in the project folder.
  • Please share this file. If you think this file contains private information, let us know, and we’ll send you a private message that will allow you to send it privately.

In any case, you can put an assert(t) after the BOOST_TEST_REQUIRE() macro. I know it’s unpleasant, but until we investigate the reproduce that’s what I can offer.

Similarly, to the BOOST_FAIL() case, you can put an assert(false) to sink that path.

5

I managed to reproduce the issue, see here.

The issue is as follows:
Both BOOST_TEST_REQUIRE and BOOST_FAIL boost macros eventually expand to a call to report_assertion(..., REQUIRE, ...).
This call is not marked noreturn, as it only aborts if REQUIRE is passed.
The body of the function is not present in the translation unit, only its declaration, so from the analyzer’s perspective this function always returns.

Because of this, neither of BOOST_TEST_REQUIRE nor BOOST_FAIL macros will sink the execution path if the condition is not met.

In your example, you don’t have a warning in the case where you use BOOST_TEST_REQUIRE, because in that case the passed t eventually escapes - meaning that its address was passed to some function via a mutable reference or pointer, hence potentially overwriting its contents.
This means that we must erase all information about t, and assume that the next call t->foo() is valid and t is nonnull.

You can also verify this theory by inserting a check against the nullness of t after the macro. That will cause us to believe that t is expected to be null in some cases (otherwise why would you check it :wink: )
So, we would end up having the same FP as in the second case.

This is a tough problem, that we might be able to tackle by some cross-translation unit information.
Basically, if we could infer that report_assertion aborts if the given parameter is REQUIRE, then we would be able to sink that path, as one expects.

About the second problem - the escape of t - it’s slightly more difficult but follows the same pattern.
If we could prove that even though the given function has a mutable reference by that parameter, it actually never mutates that object, hence we no longer need to conservatively invalidate it.

I created the CPP-4322 and CPP-4323 for conditional noreturn detection and argument escaping improvements respectively.
I wouldn’t expect to tackle these issues any time soon.

Possible workarounds:

Wrap the boost macros and insert an assert to help us. Something like this:

#define MY_BOOST_TEST_REQUIRE(cond, msg) do { BOOST_TEST_REQUIRE(static_cast<bool>(cond), msg); assert(cond); } while (0)
#define MY_BOOST_FAIL(msg) do { BOOST_FAIL(msg); assert(0); } while(0)

The escape of t could be prevented if the condition is eagerly converted to bool.
Note the static_cast<bool>(cond) in my macro. That way the address of the macro parameter can never escape, so it’s not going to be invalidated.

7

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.