[C] Detecting halt/reset

SonarQube Server / Community Build
1

Many of the findings we encounter in our C code are False Positive because the scanner does not recognize code paths that lead to a processor reset or halt. How can we annotate the code to let SonarQube know about this?

Using SonarQube 10.6

2

Hey @mek363

As a starting point, can you give an example of a rule raising a false-positive? A code sample, and the specific rule that gets triggered, would be great help.

3

Thanks for the response. Before putting together a sample, can you answer this? Would SQ recognize that this function never returns?

/**
 *  @ingroup Module_Restart
 *  @brief Perform a reset
 *
 *  @details  Reset the board. This function does not return.
 *
 *  @param mode The resulting program to run after reset.
 *
 */
void Reset(resetMode_t mode)
{
  // Due to the while (true) loop this function is not covered
  // by unit tests.

  ResetBoard(mode);

  // Ensure we don't return; This is here in case ResetBoard returns
  // unexpectedly. 
  while(true) {
  };
}
5

Hi @mek363,

Yes, the Sonar tools will recognize the Reset function as not returning; however, with very important restrictions: the body of the function needs to be visible from the point of the call for this to be used. For example, for C++ we suggest marking it with [[noreturn]]: S5271 as illustrated here.

This goes into an answer for the original question: annotating a declaration of the called function with the _Noreturn attribute is a standard way to let tools and compilers know that the function does not return.

6

Thanks for the additional information. After digging a bit I determined that far down the call chain the function that actually does the reset has this:

__attribute__((__noreturn__))

It seems like this doesn’t exactly match the syntax in the noreturn article. Do you know if this should be recognized by Sonar?

As a side note, it’s unfortunately quite a bit of work to provide a sample because of the many levels of code and macros.Are there any tools for providing examples without giving a large chunk of code?

Thanks.

7

Thanks for the additional information. After digging a bit I determined that far down the call chain the function that actually does the reset has this:

__attribute__((__noreturn__))

It seems like this doesn’t exactly match the syntax in the noreturn article. Do you know if this should be recognized by Sonar?

Yes, Sonar should recognize this attribute (see here). At this stage any further investigation will require a code sample with the information about issue that you consider as FP.

As a side note, it’s unfortunately quite a bit of work to provide a sample because of the many levels of code and macros.Are there any tools for providing examples without giving a large chunk of code?

You could generate a reproducer file for the source file that contains an issue, to generate the 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 .source file”
  • Re-run the scanner to generate a file named sonar-cfamily-reproducer.zip 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 case of an issue in a header file, you want to generate the reproducer of the source file that includes that heade
12

Tomasz: Reviving this old thread… I was able to come up with a short test case that demonstrates an issue with this. See attachment. Look for function TcpClientProcessConnection (copied below). The first assert should cause a processor reset if ctx is NULL, so ctx will never be null when the second assert is checked, yet we get a SQ finding.

void TcpClientProcessConnection(tcpClientCtx_t * ctx)
{
  ALWAYS_ASSERT_COMMON(ctx != NULL, NIMBUS_ERROR_INVALID_PARAMETER);
  ALWAYS_ASSERT_COMMON(ctx->tcpClientState == TCP_CLIENT_CONNECTING, NIMBUS_ERROR_INVALID_PARAMETER);
}

SonarQubeSandboxC.zip (2.6 KB)

16

Hello @mek363,

Currently, the analyzer works on each translation unit separately. So when it analyses TcpClientProcessConnection in TcpClientProcessConnection.c, it can see that the assert calls LogError after several macro expansions, but it does not know that this function, which is defined in reset.c, ultimately calls the function ResetBoard which is marked as __attribute__((noreturn)).

If LogError was always non-returning, a solution would be to declare LogError in a header, and decorate it with the noreturn attribute. However, this is not the case, so my suggestion would be to put in the header the first non-conditionally-noreturn function, as well as all functions leading to calling it (inline, of course). In your case, it could look like this (it will need more code to make this compile of course):

__attribute__((noreturn)) 
void Reset(resetMode_t mode); // Only the declaration

inline
void CriticalHalt(appError_t err, uint32_t cfsr, uint32_t state)
{
  int x = err + cfsr + state;
  
  // Perform a reset of the processor using the common implementation.
  if (x > 0)
    Reset(RESET_MODE_APPLICATION);
}

inline
appError_t LogError(appError_t err)
{
  if (APP_SEVERITY_IS_FAIL_HARD(APP_GET_SEVERITY(err))) {
    // If this is a critical error, go in to our halt logic. This does not
    // return
    CriticalHalt(err, 1, 2);
  }

  return err;
}

With this change, all code that includes this header will have the information needed to deduce when LogError returns and when it does not.

2 Likes
18

Thanks - that was the information I needed. I didn’t realize an attribute in one file would not be recognized when that function is referenced in another.

21

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