There’s no point discussing all the 95 warnings in this article since many of them look alike. I’ll discuss only 14 code fragments that I found worth mentioning for one reason or another.

Note. I could have well missed some warnings pointing at critical bugs. That’s why the RT-Thread developers should check the project themselves rather than rely solely on my report with those 95 warnings. I also suspect that we failed to figure out all the intricacies of RT-Thread and checked only a part of it.

Fragment №1. CWE-562: Return of Stack Variable Address

void SEMC_GetDefaultConfig(semc_config_t *config)

{

assert(config); semc_axi_queueweight_t queueWeight; /*!< AXI queue weight. */

semc_queuea_weight_t queueaWeight;

semc_queueb_weight_t queuebWeight; .... config->queueWeight.queueaWeight = &queueaWeight;

config->queueWeight.queuebWeight = &queuebWeight;

}

PVS-Studio diagnostic message: V506 CWE-562 Pointer to local variable ‘queuebWeight’ is stored outside the scope of this variable. Such a pointer will become invalid. fsl_semc.c 257

The function writes the addresses of two local variables (queueaWeight and queuebWeight) to an external structure. When control leaves the function, the variables will cease to exist but the structure will still be keeping and using the pointers to those no longer existing objects. In fact, the pointers refer to some area on the stack that may store just anything. This is a highly unpleasant security issue.

PVS-Studio reports only the last suspicious assignment, which has to do with some specifics of its inner algorithms. However, if you remove or fix the last assignment, the analyzer will be reporting the first one.

Fragment №2. CWE-570: Expression is Always False

#define CAN_FIFO0 ((uint8_t)0x00U) /*!< receive FIFO0 */

#define CAN_FIFO1 ((uint8_t)0x01U) /*!< receive FIFO1 */ uint8_t can_receive_message_length(uint32_t can_periph,

uint8_t fifo_number)

{

uint8_t val = 0U;



if(CAN_FIFO0 == fifo_number){

val = (uint8_t)(CAN_RFIFO0(can_periph) & CAN_RFIFO_RFL0_MASK);

}else if(CAN_FIFO0 == fifo_number){

val = (uint8_t)(CAN_RFIFO1(can_periph) & CAN_RFIFO_RFL0_MASK);

}else{

/* illegal parameter */

}

return val;

}

PVS-Studio diagnostic message: V517 CWE-570 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 525, 527. gd32f4xx_can.c 525

If the fifo_number argument is not equal to CAN_FIFO0, the function returns 0 all the time. It looks like the code was written using Copy-Paste and the programmer forgot to change the CAN_FIFO0 constant to CAN_FIFO1 in the cloned fragment.

Fragment №3. CWE-571: Expression is Always True

#define PECI_M0D0C_HITHR_M 0xFFFF0000 // High Threshold

#define PECI_M0D0C_LOTHR_M 0x0000FFFF // Low Threshold

#define PECI_M0D0C_HITHR_S 16

#define PECI_M0D0C_LOTHR_S 0 void

PECIDomainConfigGet(....)

{

unsigned long ulTemp;

....

ulTemp = HWREG(ulBase + PECI_O_M0D0C + (ulDomain * 4));

*pulHigh =

((ulTemp && PECI_M0D0C_HITHR_M) >> PECI_M0D0C_HITHR_S);

*pulLow =

((ulTemp && PECI_M0D0C_LOTHR_M) >> PECI_M0D0C_LOTHR_S);

}

PVS-Studio diagnostic messages:

V560 CWE-571 A part of conditional expression is always true: 0xFFFF0000. peci.c 372

V560 CWE-571 A part of conditional expression is always true: 0x0000FFFF. peci.c 373

Here we have two disappointing typos: the programmer used the && operator instead of & twice.

Because of this, the pulHigh variable will always be assigned the value 0, while the pulLow variable will be assigned 0 or 1, which is obviously not what the programmer meant this code to do.

Note for those new to the C language. The (ulTemp && PECI_M0D0C_xxxxx_M)expression always evaluates either to 0 or 1. This value, 0 or 1, is then shifted to the right. Right-shifting the value 0/1 by 16 bits will always produce 0; shifting by 0 bits will still produce 0 or 1.

Fragment №4. CWE-480: Use of Incorrect Operator

typedef enum _aipstz_peripheral_access_control {

kAIPSTZ_PeripheralAllowUntrustedMaster = 1U,

kAIPSTZ_PeripheralWriteProtected = (1U < 1),

kAIPSTZ_PeripheralRequireSupervisor = (1U < 2),

kAIPSTZ_PeripheralAllowBufferedWrite = (1U < 2)

} aipstz_peripheral_access_control_t;

PVS-Studio diagnostic messages:

V602 CWE-480 Consider inspecting the ‘(1U < 1)’ expression. ‘<’ possibly should be replaced with ‘<<’. fsl_aipstz.h 69

V602 CWE-480 Consider inspecting the ‘(1U < 2)’ expression. ‘<’ possibly should be replaced with ‘<<’. fsl_aipstz.h 70

V602 CWE-480 Consider inspecting the ‘(1U < 2)’ expression. ‘<’ possibly should be replaced with ‘<<’. fsl_aipstz.h 71

The named constants were meant to be the powers of two and store the following values: 1, 2, 4, 4. But the programmer wrote the < operator instead of << by mistake, which resulted in the following values:

kAIPSTZ_PeripheralAllowUntrustedMaster = 1

kAIPSTZ_PeripheralWriteProtected = 0

kAIPSTZ_PeripheralRequireSupervisor = 1

kAIPSTZ_PeripheralAllowBufferedWrite = 1

Fragment №5. CWE-834: Excessive Iteration

static int ft5x06_dump(void)

{

uint8_t i;

uint8_t reg_value;



DEBUG_PRINTF("[FTS] Touch Chip\r

");



for (i = 0; i <= 255; i++)

{

_ft5x06_read(i, ®_value, 1);



if (i % 8 == 7)

DEBUG_PRINTF("0x%02X = 0x%02X\r

", i, reg_value);

else

DEBUG_PRINTF("0x%02X = 0x%02X ", i, reg_value);

}

DEBUG_PRINTF("

");



return 0;

}

PVS-Studio diagnostic message: V654 CWE-834 The condition ‘i <= 255’ of loop is always true. drv_ft5x06.c 160

Variables of type uint8_t can store values within the range [0..255], so the i <= 255condition is always true. This will make the loop constantly print the debugging data.

Fragment №6. CWE-571: Expression is Always True

#define RT_CAN_MODE_NORMAL 0

#define RT_CAN_MODE_LISEN 1

#define RT_CAN_MODE_LOOPBACK 2

#define RT_CAN_MODE_LOOPBACKANLISEN 3 static rt_err_t control(struct rt_can_device *can,

int cmd, void *arg)

{

....

case RT_CAN_CMD_SET_MODE:

argval = (rt_uint32_t) arg;

if (argval != RT_CAN_MODE_NORMAL ||

argval != RT_CAN_MODE_LISEN ||

argval != RT_CAN_MODE_LOOPBACK ||

argval != RT_CAN_MODE_LOOPBACKANLISEN)

{

return RT_ERROR;

}

if (argval != can->config.mode)

{

can->config.mode = argval;

return bxcan_set_mode(pbxcan->reg, argval);

}

break;

....

}

PVS-Studio diagnostic message: V547 CWE-571 Expression is always true. Probably the ‘&&’ operator should be used here. bxcan.c 1171

The RT_CAN_CMD_SET_MODE case is never processed properly because a condition of the (x !=0 || x != 1 || x != 2 || x != 3) pattern is always true. We must be dealing with just another typo and the programmer actually meant the following:

if (argval != RT_CAN_MODE_NORMAL &&

argval != RT_CAN_MODE_LISEN &&

argval != RT_CAN_MODE_LOOPBACK &&

argval != RT_CAN_MODE_LOOPBACKANLISEN)

Fragment №7. CWE-687: Function Call With Incorrectly Specified Argument Value

void MCAN_SetSTDFilterElement(CAN_Type *base,

const mcan_frame_filter_config_t *config,

const mcan_std_filter_element_config_t *filter,

uint8_t idx)

{

uint8_t *elementAddress = 0;

elementAddress = (uint8_t *)(MCAN_GetMsgRAMBase(base) +

config->address + idx * 4U);

memcpy(elementAddress, filter, sizeof(filter));

}

The analyzer reports the error with two warnings at once:

V579 CWE-687 The memcpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. fsl_mcan.c 418

V568 It’s odd that ‘sizeof()’ operator evaluates the size of a pointer to a class, but not the size of the ‘filter’ class object. fsl_mcan.c 418

Rather than copying the whole structure of type mcan_std_filter_element_config_t, the memcpy function copies just a part of it the size of a pointer.

Fragment №8. CWE-476: NULL Pointer Dereference

There are also errors dealing with pointer dereferencing before null checks to be found in the code of RT-Thread. This is a very common bug.

static rt_size_t rt_sdcard_read(rt_device_t dev,

rt_off_t pos,

void *buffer,

rt_size_t size)

{

int i, addr;

struct dfs_partition *part =

(struct dfs_partition *)dev->user_data; if (dev == RT_NULL)

{

rt_set_errno(-EINVAL);

return 0;

}

....

}

PVS-Studio diagnostic message: V595 CWE-476 The ‘dev’ pointer was utilized before it was verified against nullptr. Check lines: 497, 499. sdcard.c 497

Fragment №9. CWE-563: Assignment to Variable without Use

static void enet_default_init(void)

{

....

reg_value = ENET_DMA_BCTL;

reg_value &= DMA_BCTL_MASK;

reg_value = ENET_ADDRESS_ALIGN_ENABLE

|ENET_ARBITRATION_RXTX_2_1

|ENET_RXDP_32BEAT |ENET_PGBL_32BEAT

|ENET_RXTX_DIFFERENT_PGBL

|ENET_FIXED_BURST_ENABLE |ENET_MIXED_BURST_DISABLE

|ENET_NORMAL_DESCRIPTOR;

ENET_DMA_BCTL = reg_value;

....

}

PVS-Studio diagnostic message: V519 CWE-563 The ‘reg_value’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3427, 3428. gd32f4xx_enet.c 3428

The reg_value = ENET_ADDRESS_ALIGN_ENABLE|…. assignment overwrites the previous value of the reg_value variable, which is strange because the variable stores the results of meaningful calculations. The code should probably look as follows:

reg_value = ENET_DMA_BCTL;

reg_value &= DMA_BCTL_MASK;

reg_value |= ENET_ADDRESS_ALIGN_ENABLE

|ENET_ARBITRATION_RXTX_2_1

|ENET_RXDP_32BEAT |ENET_PGBL_32BEAT

|ENET_RXTX_DIFFERENT_PGBL

|ENET_FIXED_BURST_ENABLE |ENET_MIXED_BURST_DISABLE

|ENET_NORMAL_DESCRIPTOR;

Fragment №10. CWE-665: Improper Initialization

typedef union _dcp_hash_block

{

uint32_t w[DCP_HASH_BLOCK_SIZE / 4];

uint8_t b[DCP_HASH_BLOCK_SIZE];

} dcp_hash_block_t; typedef struct _dcp_hash_ctx_internal

{

dcp_hash_block_t blk;

....

} dcp_hash_ctx_internal_t; status_t DCP_HASH_Init(DCP_Type *base, dcp_handle_t *handle,

dcp_hash_ctx_t *ctx, dcp_hash_algo_t algo)

{

....

dcp_hash_ctx_internal_t *ctxInternal;

....

for (i = 0; i < sizeof(ctxInternal->blk.w) /

sizeof(ctxInternal->blk.w[0]); i++)

{

ctxInternal->blk.w[0] = 0u;

}

....

}

PVS-Studio diagnostic message: V767 Suspicious access to element of ‘w’ array by a constant index inside a loop. fsl_dcp.c 946

The analyzer failed to associate this warning with any CWE ID, but it is, in fact, CWE-665: Improper Initialization.

In the loop, the value 0 is written to the 0-th element of the array all the time, while all the rest elements remain uninitialized.

Fragment №11. CWE-571: Expression is Always True

static void at91_mci_init_dma_read(struct at91_mci *mci)

{

rt_uint8_t i;

....

for (i = 0; i < 1; i++)

{

/* Check to see if this needs filling */

if (i == 0)

{

if (at91_mci_read(AT91_PDC_RCR) != 0)

{

mci_dbg("Transfer active in current

");

continue;

}

}

else {

if (at91_mci_read(AT91_PDC_RNCR) != 0)

{

mci_dbg("Transfer active in next

");

continue;

}

} length = data->blksize * data->blks;

mci_dbg("dma address = %08X, length = %d

",

data->buf, length); if (i == 0)

{

at91_mci_write(AT91_PDC_RPR, (rt_uint32_t)(data->buf));

at91_mci_write(AT91_PDC_RCR, .....);

}

else

{

at91_mci_write(AT91_PDC_RNPR, (rt_uint32_t)(data->buf));

at91_mci_write(AT91_PDC_RNCR, .....);

}

}

....

}

PVS-Studio diagnostic messages:

V547 CWE-571 Expression ‘i == 0’ is always true. at91_mci.c 196

V547 CWE-571 Expression ‘i == 0’ is always true. at91_mci.c 215

The loop body is executed exactly once, which doesn’t make sense. Why use a loop at all then?

Besides, since the i variable in the loop body remains equal to 0, some of the conditions are always true, while the rest part is never executed.

I guess the programmer actually wanted the loop body to execute twice but made a typo. The loop condition should probably look like this:

for (i = 0; i <= 1; i++)

This would make the function code meaningful.

Fragment №12. CWE-457: Use of Uninitialized Variable

Sorry for the large fragment of the function body cited below: I have to include it to prove that the k variable is really not initialized anywhere before the program reads from it.

void LCD_PutPixel (LCD_PANEL panel, uint32_t X_Left,

uint32_t Y_Up, LcdPixel_t color)

{

uint32_t k;

uint32_t * pWordData = NULL;

uint8_t* pByteData = NULL;

uint32_t bitOffset;

uint8_t* pByteSrc = (uint8_t*)&color;

uint8_t bpp = bits_per_pixel[lcd_config.lcd_bpp];

uint8_t bytes_per_pixel = bpp/8;

uint32_t start_bit;



if((X_Left >= lcd_hsize)||(Y_Up >= lcd_vsize))

return; if(panel == LCD_PANEL_UPPER)

pWordData = (uint32_t*) LPC_LCD->UPBASE +

LCD_GetWordOffset(X_Left,Y_Up);

else

pWordData = (uint32_t*) LPC_LCD->LPBASE +

LCD_GetWordOffset(X_Left,Y_Up);



bitOffset = LCD_GetBitOffset(X_Left,Y_Up);

pByteData = (uint8_t*) pWordData;

pByteData += bitOffset/8;



start_bit = bitOffset%8; if(bpp < 8)

{

uint8_t bit_pos = start_bit;

uint8_t bit_ofs = 0;

for(bit_ofs = 0;bit_ofs <bpp; bit_ofs++,bit_pos++)

{

*pByteData &= ~ (0x01 << bit_pos);

*pByteData |=

((*pByteSrc >> (k+bit_ofs)) & 0x01) << bit_pos; // <=

}

}

....

}

PVS-Studio diagnostic message: V614 CWE-457 Uninitialized variable ‘k’ used. lpc_lcd.c 510

The k variable is not initialized anywhere before being used in the expression:

*pByteData |= ((*pByteSrc >> (k+bit_ofs)) & 0x01) << bit_pos;

Fragment №13. CWE-670: Always-Incorrect Control Flow Implementation

HAL_StatusTypeDef FMC_SDRAM_SendCommand(....)

{

.... /* wait until command is send */

while(HAL_IS_BIT_SET(Device->SDSR, FMC_SDSR_BUSY))

{

/* Check for the Timeout */

if(Timeout != HAL_MAX_DELAY)

{

if((Timeout == 0)||((HAL_GetTick() - tickstart) > Timeout))

{

return HAL_TIMEOUT;

}

}



return HAL_ERROR;

}



return HAL_OK;

}

PVS-Studio diagnostic message: V612 CWE-670 An unconditional ‘return’ within a loop. stm32f7xx_ll_fmc.c 1029

The loop body executes only once at most, which looks strange since it would make more sense to use an if statement to get the same behavior. There must be some logic error here.

Fragment №14. Miscellaneous

As I already mentioned, this article covers only some of the bugs found. To see the complete list of the warnings I selected, see the HTML report (stored in the rt-thread-html-log.zip archive).

In addition to the issues that are bugs for sure, I also included the warnings pointing at suspicious code. These are the cases where I’m not sure if they are real bugs, but the RT-Thread developers should check that code out anyway. Here’s just one example.

typedef unsigned long rt_uint32_t;

static rt_err_t lpc17xx_emac_init(rt_device_t dev)

{

....

rt_uint32_t regv, tout, id1, id2;

....

LPC_EMAC->MCFG = MCFG_CLK_DIV20 | MCFG_RES_MII;

for (tout = 100; tout; tout--);

LPC_EMAC->MCFG = MCFG_CLK_DIV20;

....

}

PVS-Studio diagnostic message: V529 CWE-670 Odd semicolon ‘;’ after ‘for’ operator. emac.c 182

The programmer used the loop to introduce a small delay, which the analyzer, though indirectly, points out to us.

In the world of optimizing compilers that I’m used to, this would definitely be a bug. Compilers would simply delete this loop away to cut out any delay since tout is an ordinary, non-volatile variable. I don’t know, however, if this is true for the world of embedded systems, but I still suspect that this code is incorrect or at least unreliable. Even if the compiler doesn’t optimize such loops away, there’s no knowing how long the delay will last and if it will be long enough.

As far as I know, such systems use functions like sleep_us, and it is them that one should use for small delays. The compiler could well turn a call to sleep_us into a regular simple loop, but these are just the specifics of the implementation. When written manually, however, such delaying loops may be dangerous, not to mention the bad style.

Conclusion

I encourage you to check projects for embedded systems that you develop. It’s the first time that we have added support for the ARM compilers, so there might be some problems. So, please don’t hesitate to contact our support if you have any questions or want to report an issue.

The demo version of PVS-Studio can be downloaded here.

We understand that many projects for embedded systems are too tiny to make it worth buying a license, so we provide a free license, which is explained in the article “How to use PVS-Studio for Free”. The great advantage of our version of the free license is that you can use it not only in open-source projects but in proprietary ones as well.

Thanks for reading, and may your robots stay bugless!

References

This article will draw a new audience, so if you haven’t heard of the PVS-Studio analyzer before, you may want to check out the following articles: