The PVS-Studio 6.22 static code analyzer is adapted for ARM compilers (Keil, IAR)

PVS-Studio is a static code analyzer for finding errors and potential vulnerabilities in the code of C, C ++ and C # programs. We have long delighted readers of our blog by checking open projects and analyzing errors found. Our articles have the potential to become more interesting, as the analyzer has learned to check the code of embedded devices. We supported several ARM compilers, which you will learn more about in the article. Errors in embedded devices and robots may be more spectacular than in application programs. An error in the embedded device is not just a program crash / hang or an incorrect picture. This is a crazy Wi-Fi teapot that will boil water until it boils away and the thermal fuse blows. In general, with errors in the world of embedded-systems everything is much more interesting and worse.

My most entertaining bug

During my career as a programmer, I made a lot of mistakes in the code. However, errors in some sense were boring. Something did not work, somewhere null pointer dereferenced, and so on. Yes, these were real mistakes that required correction. However, I got the most vivid impression of my own mistake while having fun with homemade robots.

In robotics, I'm an amateur and all my crafts are purely experimental and entertaining in nature. One of the crafts was the creation of four small robots, controlled from remote controls and able to play robofootball and "catch mice." I will not go into details, I’ll just note that they knew how to: ride, hit the ball, grab claws, make sounds, blink LEDs. Actually, not to be unfounded, here is one of the robots.


')
The bot is based on an ATmega8A microcontroller (8 KB of flash, 512 bytes of EEPROM, 1 KB of RAM). In the first version of the program, one of the microcontroller timers generated an interrupt, in the handler of which commands from the remote control were read. If there were any commands, they were recorded in the FIFO buffer, from which they were then extracted and executed in the main program loop. The teams were such as: go forward / back; turn left / right; go forward, turning slightly to the left; grab the mouse; kick the ball and so on.

In fact, I have overcomplicated everything. Later, I got rid of the FIFO buffer and generally wrote more and more simply and beautifully.

Now imagine: I pour a new program into the microcontroller, turn on the robot, and ... Suddenly, the robot begins to live its own life!

Bot randomly worn on the floor, clicks the claw, pushes a nonexistent ball, flashes. Moreover, the actions are completely incomprehensible to me. The robot simply does not have a code that, in my opinion, could have caused such actions.

It was the biggest impression from an error in the program that I received for all my years of programming. It's one thing when a program crashes because of a stack overflow, and quite another when a crazy robot is worn in front of you, which you yourself did and do not even understand how this can be. It is a pity that I did not guess to shoot this action and my emotions in the background :).

After a brief trial, it turned out that I made one of the most classic programmer errors: the variable storing the number of raw commands in the FIFO buffer was not initialized. The robot began to execute a random sequence of commands, reading data from the buffer, as well as data already after the buffer.

Why did I tell this story? To simply show that errors in microcontroller programs can be more spectacular, and I hope that I can please the reader with interesting publications in the future. Now let's return to the main topic of the article devoted to the release of the new version of the PVS-Studio analyzer.

PVS-Studio 6.22

In the new version of the PVS-Studio 6.22 analyzer, our team has refined the infrastructure for testing projects of the following type:

1. Support for ARM Compiler 5 and ARM Compiler 6 has been added to Keil uVision 5.
2. The ARM Compiler 5 and ARM Compiler 6 compilers in the Keil DS-MDK environment.
3. We support IAR C / C ++ Compiler for ARM as part of the IAR Embedded Workbench environment.

Project RT-Thread

To demonstrate the new features of PVS-Studio, I needed an open project, and the choice fell on RT-Thread. This project can be built in gcc / keil / iar mode. For the purpose of additional testing of the analyzer, we checked it in both Keil mode and IAR mode. The reports were almost the same, so I don’t even remember with which of them I worked with in the future.

Now let's talk a little about the RT-Thread project itself.

RT-Thread is an open source IoT operating system from China, which has strong scalability: for example ARM Cortex-M0, or Cortex-M3 / 4/7, for a rich feature system running on MIPS32, ARM Cortex-A8, ARM Cortex-A9 DualCore etc.

Official website: rt-thread.org .

Source code: rt-thread .

I think the RT-Thread operating system is a very good candidate to become the first embedded-project tested with PVS-Studio.

Errors seen in the RT-Thread project

I skimmed the report of the PVS-Studio analyzer and selected 95 warnings representing, in my opinion, the most interesting. You can view them by downloading the rt-thread-html-log.zip archive with the full HTML report. We have recently implemented this format, and not all users know about it. Therefore, I decided to take advantage of the opportunity to re-write about it. Here’s what this report looks like in Firefox:

The report is made by analogy with the HTML report that is generated by the Clang analyzer. The report stores in itself a part of the source code, and you can immediately see to which part of the program the warnings are related. View one of the warnings as follows:

It makes no sense to consider all 95 warnings in the article, since many of them are similar. The article gives only 14 code fragments, which for some reason seemed to me worthy of description.

Note. I could miss important warnings indicating serious errors. Therefore, I ask the developers of RT-Thread not to rely only on my report containing 95 warnings, but to analyze the project myself. In addition, it seems to me that we didn’t understand how to deal with the RT-Thread project and checked only a part of it.

Fragment N1. 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 warning : V506 CWE-562 Pointer to local variable. Such a pointer will become invalid. fsl_semc.c 257

The function writes the addresses of two local variables ( queueaWeight and queuebWeight ) into the external structure. After exiting the function, the variables no longer exist, but the structure will still store and use pointers to these already non-existent objects. In fact, pointers point to some place in the stack where anything can be. This is a very nasty security bug.

The PVS-Studio analyzer reports only the last suspicious assignment, which is associated with some internal features of its work. However, if the last assignment is deleted or corrected, the analyzer will begin to warn about the first assignment.

Fragment N2. 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 warning : V517 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a possibility of logical error presence. Check lines: 525, 527. gd32f4xx_can.c 525

If the fifo_number argument is not equal to CAN_FIFO0 , then the function always returns 0. The code was most likely written using Copy-Paste and due to inattention in the copied fragment they forgot to replace the constant CAN_FIFO0 with CAN_FIFO1 .

Fragment N3. 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 warnings:

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

Two annoying typos: instead of the two & operators, the && operator was used twice.

Because of this, the value pulHigh will always be assigned the value 0, and the variable pulLow will be assigned the value 0 or 1. This is clearly not what the programmer intended.

Explanation for those who are not very familiar with the language C. The result of the expression (ulTemp && PECI_M0D0C_xxxxx_M) is always 0 or 1. Then 0 or 1 are shifted to the right. When you shift the value of 0/1 to the right by 16 bits, it will always be 0. If you shift the value of 0/1 by 0, 0 or 1 is still obtained.

Fragment N4. 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 warnings:

• V602 CWE-480 Consider inspecting the '(1U <1)' expression. '<' possibly should not be replaced with '<<'. fsl_aipstz.h 69
• V602 CWE-480 Consider inspecting the '(1U <2)' expression. '<' possibly should not be replaced with '<<'. fsl_aipstz.h 70
• V602 CWE-480 Consider inspecting the '(1U <2)' expression. '<' possibly should not be replaced with '<<'. fsl_aipstz.h 71

Named constants should have been powers of two and be equal to the following values: 1, 2, 4, 4. But instead of the operator <<, the programmer accidentally wrote <. The result is the following values:

• kAIPSTZ_PeripheralAllowUntrustedMaster = 1
• kAIPSTZ_PeripheralWriteProtected = 0
• kAIPSTZ_PeripheralRequireSupervisor = 1
• kAIPSTZ_PeripheralAllowBufferedWrite = 1

Fragment N5. CWE-834: Excessive Iteration

 static int ft5x06_dump(void) { uint8_t i; uint8_t reg_value; DEBUG_PRINTF("[FTS] Touch Chip\r\n"); for (i = 0; i <= 255; i++) { _ft5x06_read(i, &reg_value, 1); if (i % 8 == 7) DEBUG_PRINTF("0x%02X = 0x%02X\r\n", i, reg_value); else DEBUG_PRINTF("0x%02X = 0x%02X ", i, reg_value); } DEBUG_PRINTF("\n"); return 0; } 

PVS-Studio warning : V654 CWE-834 The condition 'i <= 255' of loop is always true. drv_ft5x06.c 160

Variables of type uint8_t can store values ​​in the range [0..255], so the condition i <= 255 is always true. Because of this, the loop will infinitely print debug data.

Fragment N6. 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 warning : V547 CWE-571 Expression is always true. Probably the '&&' operator should be used here. bxcan.c 1171

The case of RT_CAN_CMD_SET_MODE is always handled incorrectly. The fact is that a condition of the form (x! = 0 || x! = 1 || x! = 2 || x! = 3) is always true. Most likely, we are dealing with another typo and, in fact, it should be written here:

 if (argval != RT_CAN_MODE_NORMAL && argval != RT_CAN_MODE_LISEN && argval != RT_CAN_MODE_LOOPBACK && argval != RT_CAN_MODE_LOOPBACKANLISEN) 

Fragment N7. 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 indicates an error with two different warnings at once:

• V579 CWE-687 It is possibly a mistake. Inspect the third argument. fsl_mcan.c 418
• If you’re a sizeof () ’operator evalu evalu evalu object object object object object object fsl_mcan.c 418

The memcpy function does not copy the entire structure of type mcan_std_filter_element_config_t , but only its part equal to the size of a single pointer.

Fragment N8. CWE-476: NULL Pointer Dereference

There was no error in the RT-Thread code when the pointer is dereferenced before it is checked. This is a very common type of error.

 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 Warning: V595 CWE-476 The 'dev' pointer against it. Check lines: 497, 499. sdcard.c 497

Fragment N9. 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 warning : 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 assignment reg_value = ENET_ADDRESS_ALIGN_ENABLE | .... overwrites the previous value of the variable reg_value . This is strange, since the variable stores the result of meaningful calculations. Most likely, the code should be like this:

 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 N10. 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 warning : V767 Suspicious access to the element of the array. fsl_dcp.c 946

The analyzer was not able to associate any CWE ID with this warning, however, it should be CWE-665: Improper Initialization.

In the loop, the value 0 is written all the time in the zero element of the array, while the remaining elements remain uninitialized.

Fragment N11. 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\n"); continue; } } else { if (at91_mci_read(AT91_PDC_RNCR) != 0) { mci_dbg("Transfer active in next\n"); continue; } } length = data->blksize * data->blks; mci_dbg("dma address = %08X, length = %d\n", 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 warnings:

• 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 body of the loop runs exactly once. This makes no sense. Why, then, write a cycle at all?

Moreover, since the variable i in the body of the loop is always 0 , some conditions are always true and part of the code is never executed.

It seems to me, in fact, the developer planned that the cycle body was executed 2 times, but made a typo. Perhaps you should write a loop condition like this:

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

In this case, the function code will make sense.

Fragment N12. CWE-457: Use of Uninitialized Variable

I apologize for giving a large fragment of the function body. This is necessary to demonstrate that the variable k is not really initialized anywhere before reading a value 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 warning : V614 CWE-457 Uninitialized variable 'k' used. lpc_lcd.c 510

The variable k is not initialized anywhere until it is used in the expression:

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

Fragment N13. 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 warning : V612 CWE-670 An unconditional 'return' within a loop. stm32f7xx_ll_fmc.c 1029

The body of the loop is executed no more than once. This is very suspicious, since it would be more logical to use an if statement to get the same behavior. Most likely, there is some kind of logical error.

Fragment N14. Other

As I said earlier, I have only given some errors in the article. A complete list of the warnings I have selected can be found in the HTML report (report archive: rt-thread-html-log.zip ).

In addition to the obvious errors, I left in the report and warnings that indicate a suspicious code. I'm not sure if there is an error in the code or not, but this code should definitely be checked by the developers of RT-Thread. I will give an example of one such warning.

 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 warning : V529 CWE-670 Odd semicolon ';' after 'for' operator. emac.c 182

With the help of a loop, the programmer made a small delay. The analyzer, albeit indirectly, draws our attention to this.

In my world of optimizing compilers, this is an obvious mistake. Compilers simply throw out this cycle and there will be no delay, because tout is an ordinary non-volatile variable. I don’t know how things are in the embedded-world, but I suspect that the code is still incorrect or at least unreliable. Even if the compiler does not throw out such cycles, it is unclear how long the delay will last and whether it is sufficient.

As far as I know, in such systems there are functions like sleep_us . They should be used for small delays. The compiler may well turn the sleep_us function call into an ordinary simple cycle, but these are implementation features. Hands to write such delay cycles ugly and dangerous.

Conclusion

I invite readers to check out the projects for embedded systems in the development of which they participate. We first supported ARM compilers, and there may be overlays. Therefore, please do not hesitate and contact us in support of any questions that may arise.

You can download the demo version of PVS-Studio here .

We understand that many projects for embedded systems are very small and it would be inappropriate for them to acquire a license. Therefore, we provide a free license option, details of which can be found in the article " How to use PVS-Studio for free ." The big advantage of our free license option is the ability to use it not only in open, but also in closed projects.

Thank you all for your attention and bezbozhnyh you robots!

Additional links

This article will attract a new audience. Therefore, for those who have not yet been familiar with the PVS-Studio tool, I suggest reading the following articles:

1. Documentation. How to run PVS-Studio in Linux .
2. Andrey Karpov. Characteristics of the PVS-Studio analyzer on the example of EFL Core Libraries, 10-15% of false positives .
3. Andrey Karpov. Discussion about static code analysis .
4. Andrey Karpov. As 10 years ago, the PVS-Studio project started .
5. Andrey Karpov. Static analysis as part of the Unreal Engine development process .
6. Sergey Khrenov. PVS-Studio as a plugin for SonarQube .
7. Yevgeny Ryzhkov. The philosophy of static code analysis: we have 100 programmers, the analyzer found few errors, is it useless?
8. Sergey Vasiliev. How can PVS-Studio help in finding vulnerabilities?
9. Andrey Karpov. An article about static code analysis for managers that should not be read by programmers .
10. Andrey Karpov. How and why static analyzers deal with false positives .
11. Vsevolod Lutovinov. We build PVS-Studio into Eclipse CDT (Linux) .
12. Andrey Kuznetsov. We build PVS-Studio in Anjuta DevStudio (Linux) .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Static Code Analyzer PVS-Studio 6.22 Now Supports ARM Compilers (Keil, IAR) .