PVS-Studio - это статический анализатор кода для поиска ошибок и потенциальных уязвимостей в коде программ на языке C, C++ и C#. Мы давно радуем читателей нашего блога проверкой открытых проектов и разбором найденных ошибок. Наши статьи имеют потенциал стать более интересными, так как анализатор научился проверять код встроенных устройств. Мы поддержали несколько ARM-компиляторов, про которые подробнее вы узнаете из статьи. Ошибки во встроенных устройствах и роботах могут быть более зрелищными, чем в прикладных программах. Ошибка во встроенном устройстве - это не просто падение/зависание программы или неправильная картинка. Это сошедший с ума Wi-Fi-чайник, который будет кипятить воду, пока она не выкипит и не сработает термопредохранитель. В общем, с ошибками в мире embedded-систем всё обстоит куда интереснее и страшнее.
За свою карьеру программиста я сделал немало ошибок в коде. Однако ошибки в каком-то смысле были скучными. Что-то не так работало, где-то разыменовывался нулевой указатель и так далее. Да, это были настоящие ошибки, которые требовали исправления. Однако самое яркое впечатление от собственной ошибки я получил, развлекаясь с самодельными роботами.
В робототехнике я любитель и все мои поделки носят исключительно экспериментально-развлекательный характер. Одной из поделок стало создание четырёх небольших роботов, управляемых с дистанционных пультов и умеющих играть в робофутбол и "ловлю мышей". Не буду вдаваться в детали, отмечу только, что они умели: ездить, бить по мячику, хватать клешней, издавать звуки, мигать светодиодами. Собственно, чтобы не быть голословным, вот один из роботов (можно нажать на картинку для увеличения).
Бот реализован на базе микроконтроллера ATmega8A (8 Кбайт Flash, 512 байт EEPROM, 1 Кбайт RAM). В первом варианте программы один из таймеров микроконтроллера генерировал прерывание, в обработчике которого читались команды с пульта дистанционного управления. Если были какие-то команды, то они записывались в FIFO-буфер, из которого затем извлекались и исполнялись в главном цикле программы. Команды были такие, как: едем вперёд/назад; разворачиваемся влево/вправо; едем вперёд, поворачивая немного влево; хватаем мышку; пинаем мячик и так далее.
На самом деле, я всё переусложнил. Позже я избавился от FIFO-буфера и вообще написал всё более просто и красиво.
Теперь представьте: я заливаю в микроконтроллер новую программу, включаю робота, и... Неожиданно робот начинает жить своей собственной жизнью!
Бот хаотично носится по полу, щелкает клешнёй, толкает несуществующий мячик, мигает. Причём, действия совершенно мне непонятны. В роботе просто нет кода, который, на мой взгляд, мог бы стать причиной таких действий.
Это было самое большое впечатление от ошибки в программе, полученное мною за все мои годы программирования. Одно дело, когда программа падает из-за переполнения стека, и совсем другое, когда перед тобой носится чокнутый робот, которого ты сам сделал и даже не понимаешь, как такое может быть. Жаль, что я не догадался заснять это действо и свои эмоции на заднем плане :).
После недолгого разбирательства выяснилась, что я допустил одну из самых классических программистских ошибок: переменная, хранящая количество необработанных команд в FIFO-буфере, оказалась неинициализированной. Робот начал выполнять случайную последовательность команд, читая данные из буфера, а также данные, находящиеся уже после буфера.
Зачем я рассказал эту историю? Чтобы просто показать, что ошибки в программах микроконтроллеров могут быть более зрелищными, и я надеюсь, что смогу порадовать в будущем читателя интересными публикациями. Теперь же давайте вернемся к основной теме статьи, посвященной выходу новой версии анализатора PVS-Studio.
В новой версии анализатора PVS-Studio 6.22 наша команда доработала инфраструктуру для проверки проектов следующего типа:
Для демонстрации новых возможностей PVS-Studio мне понадобился открытый проект, и выбор пал на RT-Thread. Этот проект можно собрать в режиме gcc/keil/iar. С целью дополнительного тестирования анализатора мы проверили его как в режиме Keil, так и в режиме IAR. Отчёты получились практически одинаковыми, так что я даже не помню, с каким из них я в дальнейшем работал.
Давайте теперь немного поговорим о самом проекте RT-Thread.
RT-Thread is an open source IoT operating system from China, which has strong scalability: from a tiny kernel running on a tiny core, for example ARM Cortex-M0, or Cortex-M3/4/7, to a rich feature system running on MIPS32, ARM Cortex-A8, ARM Cortex-A9 DualCore etc.
Официальный сайт: rt-thread.org.
Исходный код: rt-thread.
Думаю, операционная система RT-Thread - очень хороший кандидат, чтобы стать первым embedded-проектом, проверенным с помощью PVS-Studio.
Я бегло просмотрел отчёт анализатора PVS-Studio и отобрал 95 предупреждений, представляющих, на мой взгляд, наибольший интерес. Вы можете ознакомиться с ними, скачав архив rt-thread-html-log.zip с полным HTML-отчётом. Этот формат мы реализовали недавно, и не все пользователи знают про него. Поэтому я решил воспользоваться подходящей возможностью, чтобы вновь написать о нём. Вот как выглядит этот отчёт, открытый в Firefox (нажмите на изображение для увеличения):
Отчёт сделан по аналогии с HTML отчётом, который генерирует анализатор Clang. Отчёт хранит в себе часть исходного кода, и вы можете сразу увидеть, к какому фрагменту программы относятся предупреждения. Просмотр одного из предупреждений выглядит следующим образом (нажмите на картинку для увеличения):
Нет смысла рассматривать в статье все 95 предупреждений, так как многие из них похожи. В статье приведу только 14 фрагментов кода, которые мне показались по какой-то причине заслуживающими описания.
Примечание. Я мог пропустить важные предупреждения, указывающие на серьёзные ошибки. Поэтому прошу разработчиков RT-Thread не полагаться только на мой отчёт, содержащий 95 предупреждений, а провести анализ проекта самостоятельно. Вдобавок мне кажется, мы не разобрались как следует с проектом RT-Thread и проверили только какую-то его часть.
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: 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
Функция записывает во внешнюю структуру адреса двух локальных переменных (queueaWeight и queuebWeight). После выхода из функции переменные перестают существовать, но структура по-прежнему будет хранить и использовать указатели на эти уже несуществующие объекты. Фактически, указатели указывают в какое-то место стека, в котором может быть всё что угодно. Это очень неприятная ошибка безопасности.
Анализатор PVS-Studio сообщает только о последнем подозрительном присваивании, что связано с некоторыми внутренними особенностями его работы. Однако, если последнее присваивание удалить или исправить, то анализатор начнёт предупреждать о первом присваивании.
#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: 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
Если аргумент fifo_number не равен CAN_FIFO0, то функция всегда возвращает 0. Код, скорее всего, писался с помощью Copy-Paste и по невнимательности в скопированном фрагменте забыли заменить константу CAN_FIFO0 на CAN_FIFO1.
#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:
Две досадные опечатки: вместо двух операторов & дважды использовали оператор &&.
Из-за этого переменной pulHigh всегда будет присваиваться значение 0, а переменной pulLow будет присвоено значение 0 или 1. Это явно не то, что задумывал программист.
Пояснение для тех, кто слабо знаком с языком C. Результатом выражения (ulTemp && PECI_M0D0C_xxxxx_M) всегда является 0 или 1. Далее 0 или 1 сдвигаются вправо. При сдвиге вправо значения 0/1 на 16 бит всегда будет получаться 0. При сдвиге значения 0/1 на 0 бит по-прежнему получается 0 или 1.
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:
Именованные константы должны были являться степенями двойки и быть равны следующим значениям: 1, 2, 4, 4. Но вместо оператора << программист случайно написал <. В результате получаются следующие значения:
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, ®_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: V654 CWE-834 The condition 'i <= 255' of loop is always true. drv_ft5x06.c 160
Переменные типа uint8_t могут хранить значения в диапазоне [0..255], поэтому условие i <= 255 всегда истинно. Из-за этого цикл будет бесконечно распечатывать отладочные данные.
#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: V547 CWE-571 Expression is always true. Probably the '&&' operator should be used here. bxcan.c 1171
Случай RT_CAN_CMD_SET_MODE всегда обрабатывается неверно. Дело в том, что условие вида (x !=0 || x != 1 || x != 2 || x != 3) всегда является истинным. Скорее всего, мы имеем дело с очередной опечаткой и, на самом деле, здесь должно быть написано:
if (argval != RT_CAN_MODE_NORMAL &&
argval != RT_CAN_MODE_LISEN &&
argval != RT_CAN_MODE_LOOPBACK &&
argval != RT_CAN_MODE_LOOPBACKANLISEN)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));
}Анализатор указывает на ошибку сразу двумя разными предупреждениями:
Функция memcpy копирует не всю структуру типа mcan_std_filter_element_config_t, а только её часть, равную размеру одного указателя.
Не обошлось в коде RT-Thread без ошибок, когда указатель разыменовывается до его проверки. Это очень распространённый тип ошибки.
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: V595 CWE-476 The 'dev' pointer was utilized before it was verified against nullptr. Check lines: 497, 499. sdcard.c 497
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: 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
Присваивание reg_value = ENET_ADDRESS_ALIGN_ENABLE|.... перетирает предыдущее значение переменной reg_value. Это странно, так как в переменной хранится результат осмысленных вычисления. Скорее всего, код должен быть таким:
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;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: V767 Suspicious access to element of 'w' array by a constant index inside a loop. fsl_dcp.c 946
Анализатор не смог сопоставить с этим предупреждением никакой CWE ID, однако по смыслу это должно быть CWE-665: Improper Initialization.
В цикле значение 0 записывается всё время в нулевой элемент массива, в то время как остальные элементы остаются неинициализированными.
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:
Тело цикла выполняется ровно один раз. Это не имеет смысла. Зачем тогда вообще писать цикл?
Более того, так как в теле цикла переменная i всегда равна 0, то некоторые условия всегда истинные и часть кода никогда не выполняется.
Мне кажется, на самом деле, разработчик планировал, чтобы тело цикла выполнялось 2 раза, но допустил опечатку. Возможно, следовало написать условие цикла так:
for (i = 0; i <= 1; i++)В этом случае код функции обретёт смысл.
Прощу прощения, что приведу большой фрагмент тела функции. Это необходимо, чтобы продемонстрировать, что переменная k действительно нигде не инициализируется перед чтением из неё значения.
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: V614 CWE-457 Uninitialized variable 'k' used. lpc_lcd.c 510
Переменная k нигде не инициализируется до момента её использования в выражении:
*pByteData |= ((*pByteSrc >> (k+bit_ofs)) & 0x01) << bit_pos;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: V612 CWE-670 An unconditional 'return' within a loop. stm32f7xx_ll_fmc.c 1029
Тело цикла выполняется не более одного раза. Это очень подозрительно, так как для получения такого же поведения логичнее было бы использовать оператор if. Скорее всего, здесь какая-то логическая ошибка.
Как я уже говорил ранее, я привёл в статье только некоторые ошибки. Полный список выбранных мною предупреждений можно найти в HTML-отчёте (архив с отчётом: rt-thread-html-log.zip).
Помимо явных ошибок, я оставил в отчёте и предупреждения, которые указывают на подозрительный код. Я не уверен, есть в коде ошибка или нет, но этот код однозначно следует проверить разработчикам RT-Thread. Приведу пример одного такого предупреждения.
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: V529 CWE-670 Odd semicolon ';' after 'for' operator. emac.c 182
С помощью цикла программист осуществил маленькую задержку. Анализатор, пусть и косвенно, обращает на это наше внимание.
В моём мире оптимизирующих компиляторов это явная ошибка. Компиляторы просто выкинут этот цикл и никакой задержки не будет, т.к. tout - это обыкновенная не volatile переменная. Как дело обстоит в embedded-мире я не знаю, но подозреваю, что код всё равно некорректен или, по крайней мере, ненадёжен. Даже если компилятор не выкидывает такие циклы, непонятно, сколько по времени будет длиться задержка и будет ли она достаточна.
Насколько я знаю, в подобных системах есть функции типа sleep_us. Их и следует использовать для маленьких задержек. Компилятор вполне может превратить вызов функции sleep_us в обыкновенный простой цикл, но это уже особенности реализации. Руками же писать такие циклы задержки некрасиво и опасно.
Приглашаю читателей проверить проекты для встраиваемых систем, в разработке которых они принимают участие. Мы впервые поддержали ARM-компиляторы, и могут быть накладки. Поэтому прошу не стесняться и по всем возникшим вопросам обращаться к нам в поддержку.
Вы можете скачать демонстрационную версию PVS-Studio здесь.
Мы понимаем, что многие проекты для встраиваемых систем являются совсем маленькими и для них окажется нецелесообразным приобретать лицензию. Поэтому мы предоставляем вариант бесплатной лицензии, подробности о которой можно узнать из статьи "Как использовать PVS-Studio бесплатно". Большим преимуществом нашего варианта бесплатной лицензии является возможность использовать её не только в открытых, но и в закрытых проектах.
Спасибо всем за внимание и безбажных вам роботов!
Эта статья привлечёт новую аудиторию. Поэтому тем, кто ещё не был знаком с инструментом PVS-Studio, предлагаю ознакомиться со следующими статьями: