Warning from Clang Static Analyzer about returning null reference

[nlohmann]

Description

I ran Clang Static Analyzer over nlohmann/json which uses Catch 2.2.1 in a feature branch. The analyzer reported a warning in Catch's source code:

/Users/niels/Documents/repositories/json/test/thirdparty/catch/catch.hpp:4620:9: warning: Returning null reference
        return *IMutableContext::currentContext;
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/niels/Documents/repositories/json/test/thirdparty/catch/catch.hpp:9341:17: warning: Potential leak of memory pointed to by 'filename.m_start'
                CATCH_ENFORCE( !m_ofs.fail(), "Unable to open file: '" << filename << "'" );
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/niels/Documents/repositories/json/test/thirdparty/catch/catch.hpp:6620:56: note: expanded from macro 'CATCH_ENFORCE'
    do{ if( !(condition) ) CATCH_ERROR( msg ); } while(false)
                                                       ^~~~~

I attached a ZIP file of the HTML report where the analyzer's trace can be followed: report.zip

This may be a false positive, but I though this about all warnings of Clang Static Analyzer that eventually turned out to be real issues :-)

Steps to reproduce

• check out the feature/catch2 branch of nlohmann/json
• run make clang_analyze

(possibly adjust the path to Clang Static Analyzer).

Extra information

• Catch version: v2.2.1
• Operating System: macOS High Sierra 10.13.3 (17D102)
• Compiler+version: clang version 5.0.0 (trunk 292523)

[horenmar]

The second warning is definitely bogus, but I'll take a look at the null deref.

Btw, looking through the changes, you might be interested in REQUIRE_THROWS_MATCHES(expr, exception-type, matcher).

[horenmar]

Finally got around this and it is definitely a false positive.

The analyzer suggests that the problem occurs when IMutableContext::currentContext is a nullptr at line 4619 in function getCurrentMutableContext(). However it does not look through the single function call in that branch, IMutableContext::createContext(), which does this

currentContext = new Context();

Obviously, there is no way to have the currentContext still be nullptr after this function call ends.

[horenmar]

As there has been no followup and, as far as I can tell, there is no issue, I am going to close this.

[nlohmann]

Thanks for checking. Indeed there I can't follow up on this. Keep up the good work on Catch! 👍

[martin-ejdestig]

Reported https://bugs.llvm.org/show_bug.cgi?id=39201 upstream.

[martin-ejdestig]

It still reproduces with Clang 7.0 and Catch2 2.4.1.

[FabioFracassi]

This still happens with clang-8.0 and clang build from master, with catch 2.5 and 2.7.
You can suppress this warning by adding // NOLINT(clang-analyzer-core.uninitialized.UndefReturn) after the return statement (or //NOLINTNEXTLINE(clang-analyzer-core.uninitialized.UndefReturn) before).

You could probably also "fix" it by using function local statics, which has the nice side effect of making it thread safe (since C++11)

inline IMutableContext& getCurrentMutableContext() {
        static IMutableContext instance{}; // default construct context exactly once
        return instance;
}