Why is this line of code not reported as a bug?

SonarQube Server / Community Build
,
1

Hi,

I don’t understand why BufferData[PtrBufferData++] is not reported as a bug (see code below)? It is a real-life bug that I had to correct (PtrBufferData was overflowing the buffer), but I can see that SonarQube didn’t report it at all (not even as Code Smell).

static __xdata UINT8 BufferData[1024] ;
static __xdata UINT16 PtrBufferData ;

void function(void)
{
	switch (step) {
		case DATABLOCK:
			BufferData[PtrBufferData++] = CarRecuPCSystem ;
			ChkSum ^= CarRecuPCSystem ;
			if (PtrBufferData == NbOctetData) {
				step = PRT1 ;
			}
		break ;

Shouldn’t this easy to spot by the static analysis tool?

SonarQube 7.9.1
C code build with Sonar build wrapper + IAR compiler

2

Hello @smainier and welcome to our community!

I suppose you were expecting this rule Memory access should be explicitly bounded to prevent buffer overflows to be triggered.
This rule is based on symbolic-execution, i.e. it tries to reason about the state of the program to detect bugs. Unfortunately this technology is not perfect and can miss some cases, like yours apparently.

However, we always try to improve this so thank you for reporting this false negative!
Could you share with us a bit more information about this false negative so that we can reproduce it? From what you shared, PtrBufferData is uninitialized but you say that it was overflowing the buffer. Is it initialized in a previous statement?

Thanks

3

Amelie,

Thanks for looking at this.

The function where I found the bug is a function that decodes a stream of data arriving on a serial communication bus. Messages have a variable length (NbBytesData) and that length is derived from the 1st bytes arriving on the bug after a synchronization token.

If the decoded length was correct, this function was working fine, but in case the decoded length was wrong (corruption of the data?), the variable NbBytesData would be wrong and thus the statement BufferData[PtrBufferData++] = byte_received finishes by overflowing the buffer BufferData.

Here is a longer code snippet that should help to understand why the rule didn’t trigger an error in that case:

static __xdata UINT16 NbBytesData ;	
static __xdata UINT16 NbBytesMessage ;
static __xdata UINT8 BufferData[1024] ;
static __xdata UINT16 PtrBufferData ;

void decode_data_stream(void)
{
	switch (step) {
		case FIND_TOKEN:
			if (/* token found*/) 
            {
                step = DECODE_LENGTH_1;
            }
		break ;
		case DECODE_LENGTH_1:
			NbBytesMessage = byte_received ;
			step = DECODE_LENGTH_2 ;
		break ;
		case DECODE_LENGTH_2:
			NbBytesMessage += (byte_received << 8) ;
			NbBytesData = NbBytesMessage - NB_UNUSED_CHAR ;	
			PtrBufferData = 0 ;	// init of index in buffer
			step = DECODE_NEXT_BYTES ;
		break ;
		case DECODE_NEXT_BYTES:
			/* ... */
            if (NbBytesData == 0) {
				step = DECODE_FINAL_BYTES ;
			} else {
				step = DATABLOCK ;
			}
		break ;
		case DATABLOCK:
			BufferData[PtrBufferData++] = byte_received ;

			if (PtrBufferData == NbBytesData) {
				step = DECODE_FINAL_BYTES ;
			}
		break ;
		case DECODE_FINAL_BYTES:
			/* ... */
            step = FIND_TOKEN
		break ;
	}
}

Let me know if you understand why we had a false positive.

Thanks

4

One more precision, the function decode_data_stream is called for each byte received on the communication bus.

5

Hello @smainier ,

Thanks for the explanation, that is really helpful.

However, in the case you describe, I don’t see how the analyzer could know that the value of NbBytesData is wrong. If I understand correctly, NbBytesData is initialized during runtime, when the communication bus is decoded. But our analyzer relies on static analysis, i.e. a method to debug source code before it runs.
So, because the analyzer doesn’t know the value of NbBytesData (so does not know that it is wrong), it does not raise any issue (if it raised issues each time it does not know a value, the number of raised issues would be huge and that would not be a pleasant experience at all)

Does it make sense to you?

6

It makes sense the analyzer cannot say for sure it is a bug. But maybe that should be enough for a Code Smell: no clear check of the index PtrBufferData value before we use it for changing values in BufferData. I was really surprised that the analyzer didn’t mention anything.

1 Like
7

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