(SOLVED) Possible false positive due to assumption of failing assert

[JWRWSEU (Jack White)]

We are on SQ 10.3, running on an internal server. I have the following code:

Result
myFunction(uint8_t state_index)
{
    Result      result       = E_RESULT_OK;
    OtherResult other_result = E_OTHER_RESULT_OK;

    other_result = doStuffAndThings();

    if (E_OTHER_RESULT_OK != other_result)
    {
        result = E_RESULT_INTERNAL_ERROR;

        simple_err_payload[0] = (uint8_t) E_ENUM_VAL;
        simple_err_payload[1] = state_index;
        simple_err_payload[2] = (uint8_t) other_result;

        Error_send(
            ID1,
            ID2,
            simple_err_payload,
            sizeof(simple_err_payload),
            E_DESTINATION_1);
        
        /* Stop execution during functional testing because this is a
         * critical integration failure
         */
        FATAL_FAILURE;
    }

    return result;
}

SonarQube says, “Value stored to ‘result’ is never read” and points to c:S1854 “Unused assignments should be removed.”

The only thing I can see which would cause this, because Error_send() cannot fail, is the FATAL_FAILURE macro, which is assert(false) behind the scenes. But when building for the SonarQube scan, asserts are off.

Am I going mad? Does anyone else have this problem?

[aalvarez (Alejandro Álvarez Ayllón)]

Hello @JWRWSEU ,

@JWRWSEU

The only thing I can see that would cause this, because Error_send() cannot fail, is the FATAL_FAILURE macro, which is assert(false) behind the scenes. But when building for the SonarQube scan, asserts are off.

You are correct in your deduction. We leverage assert during the analysis even if NDEBUG is defined.

Here there is a small reproducer where you can see Sonar does not report a null pointer dereference even when NDEBUG is defined, while the actual execution segfaults.

This is because asserts are considered to be used to verify invariants, not to report errors (regardless of the gravity of the error).
Hence, assert(false) is interpreted as “we can never reach this statement; if we do, we have undefined behavior”. And undefined behavior means the program is invalid (it can not be reasoned about).

The only formal interpretation left is that Error_send never returns, and the value stored with result = E_RESULT_INTERNAL_ERROR is never read. The analyzer looks at the formal logical meaning.

A more pragmatic way of thinking about this: if a statement that must not execute executes, perhaps the stack has been smashed, or the memory corrupted, and we can not know which state the program is in.

A way of definitely removing the assert so it is not seen and not interpreted as an invariant would be to define FATAL_FAILURE itself conditionally:

#ifdef NDEBUG
#define FATAL_FAILURE
#else
#define FATAL_FAILURE assert(false)
#endif

I hope this helps.