По заказам Embedded-разработчиков: ищем ошибки в Amazon FreeRTOS
Каждый, кто программирует микроконтроллеры, наверняка знает о FreeRTOS, или по крайней мере слышал об этой операционной системе. Ребята из Amazon решили расширить возможности этой операционной системы для работы с сервисами AWS Internet of Things — так появилась Amazon FreeRTOS. Нас, разработчиков анализатора кода PVS-Studio, в почте и в комментариях под статьями попросили проверить эти проекты. Что ж, вы просили — мы сделали. Что из этого получилось — читайте далее.
Немного о проектах
Для начала расскажу немного о «папе» проверяемого проекта — FreeRTOS (исходный код вы можете найти по ссылке). Как говорит Википедия, FreeRTOS — это многозадачная операционная система реального времени для встраиваемых систем.
Написана она на старом добром Си, что неудивительно — данная операционная система должна работать в условиях, типичных для микроконтроллеров: невысокая вычислительная мощность, небольшой объем оперативной памяти, и тому подобное. Язык Си позволяет работать с ресурсами на низком уровне и имеет высокую производительность, поэтому он как нельзя лучше подходит для разработки такой ОС.
Теперь вернемся к компании Amazon, которая не сидит на месте и развивается по различным перспективным направлениям. Например, в Amazon разрабатывается игровой AAA-движок Amazon Lumberyard, который мы тоже проверяли.
Одним из таких направлений является Internet of Things (интернет вещей, IoT). Для развития в этой сфере в Amazon решили написать свою операционную систему — и за основу они взяли ядро FreeRTOS.
Получившаяся система — Amazon FreeRTOS — позиционируется как «обеспечивающая возможность безопасного подключения к сервисам Amazon Web Services, таким как AWS IoT Core или AWS IoT Greengrass». Исходный код этого проекта хранится на Github.
В этой статье мы рассмотрим, имеются ли ошибки во FreeRTOS, а также то, насколько безопасна операционная система от Amazon с точки зрения статического анализа кода.
Как проводилась проверка
Проверка кода проводилась с помощью автоматического средства поиска ошибок: статическим анализатором кода PVS-Studio. Он способен выявлять ошибки в программах, написанных на языках C, C++, C# и Java.
Перед началом анализа необходимо собрать проект — так я буду уверен, что у меня имеются все необходимые зависимости и с проектом всё в порядке. Проверять проект можно несколькими способами — например, с помощью системы мониторинга компиляции. Я же проводил анализ с помощью плагина для Visual Studio — хорошо, что в репозиториях обоих проектов имеется набор проектных файлов, позволяющих легко провести сборку под Windows.
Мне было достаточно лишь собрать проекты, чтобы убедиться, что есть всё необходимое для проверки. Далее я запустил анализ и вуаля! — передо мной готовый отчёт анализатора.
Сторонние библиотеки, включенные в эти проекты, тоже могут содержать ошибки, и они, конечно, тоже могут влиять на работу программы. Тем не менее, я исключил их из анализа ради чистоты повествования.
Итак, проекты проанализированы, отчеты получены, интересные ошибки выписаны. Пора перейти к их разбору!
Что скрывает FreeRTOS
Изначально я рассчитывал написать две отдельных статьи: по одной на каждую операционную систему. Я уже потирал руки, готовясь написать хорошую статью про FreeRTOS. Предвкушая обнаружение хотя бы парочки сочных ошибок (вроде CWE-457), я с нетерпением просматривал немногочисленные предупреждения анализатора, и… и ничего. Я не нашел ни одной интересной ошибки.
Многие предупреждения, которые выдал анализатор, были не актуальны для FreeRTOS. Например, такими предупреждениями были 64-битные недочёты вроде приведения size_t к uint32_t. Это связано с тем, что FreeRTOS рассчитана для работы на устройствах с размером указателя не больше, чем 32 бит.
Я тщательно проверил все предупреждения V1027, связанные с приведениями между указателями на несвязанные между собой структуры. Если приводимые структуры имеют одинаковое выравнивание — значит, такое приведениене является ошибкой. И я не нашел ни одного опасного приведения!
Все остальные подозрительные места были либо связаны со стилем кодирования, либо были укомплектованы комментарием, объясняющим, почему здесь сделано именно так, и почему это не является ошибкой.
В общем, хочу обратиться к разработчикам FreeRTOS. Ребята, вы настоящие молодцы! Таких чистых и качественных проектов, как ваш, мы практически не встречали. И мне было очень приятно почитать чистый, опрятный, и хорошо задокументированный код. Снимаю перед вами шляпу.
Хоть я и не смог найти интересных ошибок в тот день, я понимал, что на этом я не остановлюсь. Я шел домой с твердой уверенностью, что в версии от Amazon 100% обнаружится что-нибудь интересное, и что завтра я обязательно соберу достаточно ошибок для статьи. Как вы уже наверняка догадались, я оказался прав.
Что скрывает Amazon FreeRTOS
Версия системы от Amazon оказалась… мягко скажем, чуть хуже. Наследие FreeRTOS осталось таким же чистым, а вот в новых доработках оказалось немало интересного.
В некоторых местах была нарушена логика программы, где-то неправильно работали с указателями. Местами код мог привести к неопределенному поведению, а где-то программист просто не знал о паттерне ошибки, которую он допустил. Я даже нашел несколько серьезных потенциальных уязвимостей.
Что-то я затянул со вступлением. Давайте начнем разбирать ошибки!
Нарушение логики программы
Начнем с проблемных мест, которые явно указывают, что программа выполняется не совсем так, как рассчитывал программист. Первым таким местом будет подозрительная работа с массивом:
/**
* @brief Pool of request and associated response buffers,
* handles, and configurations.
*/
static _requestPool_t _requestPool = { 0 };
....
static int _scheduleAsyncRequest(int reqIndex,
uint32_t currentRange)
{
....
/* Set the user private data to use in the asynchronous callback context.
*/
_requestPool.pRequestDatas[reqIndex].pConnHandle = &_connHandle;
_requestPool.pRequestDatas[reqIndex].pConnConfig = &_connConfig;
_requestPool.pRequestDatas[reqIndex].reqNum = reqIndex;
_requestPool.pRequestDatas[reqIndex].currRange = currentRange;
_requestPool.pRequestDatas[reqIndex].currDownloaded = 0;
_requestPool.pRequestDatas[reqIndex].numReqBytes = numReqBytes;
....
_requestPool.pRequestDatas->scheduled = true;
....
}
PVS-Studio выдал два предупреждения на этот фрагмент кода:
- V619 The array '_requestPool.pRequestDatas' is being utilized as a pointer to single object. iot_demo_https_s3_download_async.c 973
- V574 The '_requestPool.pRequestDatas' pointer is used simultaneously as an array and as a pointer to single object. Check lines: 931, 973. iot_demo_https_s3_download_async.c 973
На всякий случай напомню: имя массива является указателем на его первый элемент. То есть, если _requestPool.pRequestDatas — это массив структур, то _requestPool.pRequestDatas[i].scheduled — это доступ к члену scheduled i-той структуры массива. А если написать _requestPool.pRequestDatas→scheduled, то это будет означать доступ к члену scheduled первой структуры массива.
В отрывке кода, приведенном выше, так и происходит. В последней строке всегда изменяется значение только для члена первой структуры массива. Само по себе такое обращение уже подозрительно, но здесь ситуация еще более явная: на протяжении всего тела функции к массиву _requestPool.pRequestDatas обращаются по индексу, и только в конце операцию индексирования забыли применить.
Как я понимаю, последняя строчка должна выглядеть так:
_requestPool.pRequestDatas[reqIndex].scheduled = true;
Следующая ошибка кроется в маленькой функции, поэтому приведу её целиком:
/* Return true if the string " pcString" is found
* inside the token pxTok in JSON file pcJson. */
static BaseType_t prvGGDJsoneq( const char * pcJson,
const jsmntok_t * const pxTok,
const char * pcString )
{
uint32_t ulStringSize = ( uint32_t ) pxTok->end
- ( uint32_t ) pxTok->start;
BaseType_t xStatus = pdFALSE;
if( pxTok->type == JSMN_STRING )
{
if( ( uint32_t ) strlen( pcString ) == ulStringSize )
{
if( ( int16_t ) strncmp( &pcJson[ pxTok->start ], // <=
pcString,
ulStringSize ) == 0 )
{
xStatus = pdTRUE;
}
}
}
return xStatus;
}
Предупреждение PVS-Studio: V642 [CWE-197] Saving the 'strncmp' function result inside the 'short' type variable is inappropriate. The significant bits could be lost breaking the program’s logic. aws_greengrass_discovery.c 637
Давайте заглянем в определение функции strncmp:
int strncmp( const char *lhs, const char *rhs, size_t count );
В примере результат типа int, размер которого равен 32 битам, конвертируется в переменную типа int16_t. При такой «сужающей» конвертации будут потеряны старшие биты возвращаемого значения. Например, если функция strncmp вернет 0×00010000, то при конвертации «единичка» потеряется, и условие выполнится.
На самом деле странно видеть такое приведение в условии. Зачем вообще его делать, если с нулём можно сравнить и обычный int? С другой стороны, если программист осознанно хотел, чтобы функция иногда возвращала true, даже если не должна этого делать — то почему такое хитрое поведение не описано комментарием? Но тогда это уже какая-то закладка получается. В общем, я склоняюсь к тому, что это ошибка. А как считаете вы?
Неопределенное поведение и указатели
Сейчас будет довольно большой пример. Он таит в себе потенциальное разыменование нулевого указателя:
static void _networkReceiveCallback(....)
{
IotHttpsReturnCode_t status = IOT_HTTPS_OK;
_httpsResponse_t* pCurrentHttpsResponse = NULL;
IotLink_t* pQItem = NULL;
....
/* Get the response from the response queue. */
IotMutex_Lock(&(pHttpsConnection->connectionMutex));
pQItem = IotDeQueue_PeekHead(&(pHttpsConnection->respQ));
IotMutex_Unlock(&(pHttpsConnection->connectionMutex));
/* If the receive callback is invoked
* and there is no response expected,
* then this a violation of the HTTP/1.1 protocol. */
if (pQItem == NULL)
{
IotLogError(....);
fatalDisconnect = true;
status = IOT_HTTPS_NETWORK_ERROR;
goto iotCleanup;
}
....
iotCleanup :
/* Report errors back to the application. */
if (status != IOT_HTTPS_OK)
{
if ( pCurrentHttpsResponse->isAsync
&& pCurrentHttpsResponse->pCallbacks->errorCallback)
{
pCurrentHttpsResponse->pCallbacks->errorCallback(....);
}
pCurrentHttpsResponse->syncStatus = status;
}
....
}
Предупреждение PVS-Studio: V522 [CWE-690] There might be dereferencing of a potential null pointer 'pCurrentHttpsResponse'. iot_https_client.c 1184
Проблемные разыменования находятся в самом нижнем if. Давайте же разберемся, что здесь происходит.
В начале функции переменные pCurrentHttpsResponse и pQItem инициализируются значением NULL, а переменная status — значением IOT_HTTPS_OK, означающим, что все проходит штатно.
Далее в pQItem присваивается значение, возвращенное из функции IotDeQueue_PeekHead, которая возвращает указатель на начало двусвязной очереди.
Что произойдет, если очередь окажется пустая? В этом случает функция IotDeQueue_PeekHead вернет NULL:
static inline IotLink_t* IotDeQueue_PeekHead
(const IotDeQueue_t* const pQueue)
{
return IotListDouble_PeekHead(pQueue);
}
....
static inline IotLink_t* IotListDouble_PeekHead
(const IotListDouble_t* const pList)
/* @[declare_linear_containers_list_double_peekhead] */
{
IotLink_t* pHead = NULL;
if (pList != NULL)
{
if (IotListDouble_IsEmpty(pList) == false)
{
pHead = pList->pNext;
}
}
return pHead;
}
Далее выполнится условие pQItem == NULL, и поток управления перейдет по goto в нижнюю часть тела функции. К этому времени указатель pCurrentHttpsResponse так и останется нулевым, а status уже не будет равен IOT_HTTPS_OK. В итоге мы попадем в ту самую ветвь if, и… бабах! Последствия такого разыменования вы и сами знаете.
Окей. Это был слегка витиеватый пример. Теперь предлагаю вашему вниманию очень простое и понятное потенциальное разыменование:
int PKI_mbedTLSSignatureToPkcs11Signature
(uint8_t * pxSignaturePKCS, uint8_t * pxMbedSignature )
{
int xReturn = 0;
uint8_t * pxNextLength;
/* The 4th byte contains the length of the R component */
uint8_t ucSigComponentLength = pxMbedSignature[ 3 ]; // <=
if( ( pxSignaturePKCS == NULL )
|| ( pxMbedSignature == NULL ) )
{
xReturn = FAILURE;
}
....
}
Предупреждение PVS-Studio: V595 [CWE-476] The 'pxMbedSignature' pointer was utilized before it was verified against nullptr. Check lines: 52, 54. iot_pki_utils.c 52
Эта функция получает два указателя на uint8_t. Оба указателя проверяются на NULL, что является хорошей практикой — такие ситуации надо отрабатывать сразу.
Но вот незадача: к моменту, когда будет проверяться pxMbedSignature, он будет уже разыменован буквально одной строкой выше. Та-даа!
Еще один пример умозрительного кода:
CK_RV vAppendSHA256AlgorithmIdentifierSequence
( uint8_t * x32ByteHashedMessage,
uint8_t * x51ByteHashOidBuffer )
{
CK_RV xResult = CKR_OK;
uint8_t xOidSequence[] = pkcs11STUFF_APPENDED_TO_RSA_SIG;
if( ( x32ByteHashedMessage == NULL )
|| ( x51ByteHashOidBuffer == NULL ) )
{
xResult = CKR_ARGUMENTS_BAD;
}
memcpy( x51ByteHashOidBuffer,
xOidSequence,
sizeof( xOidSequence ) );
memcpy( &x51ByteHashOidBuffer[ sizeof( xOidSequence ) ],
x32ByteHashedMessage,
32 );
return xResult;
}
Предупреждения PVS-Studio:
- V1004 [CWE-628] The 'x51ByteHashOidBuffer' pointer was used unsafely after it was verified against nullptr. Check lines: 275, 280. iot_pkcs11.c 280
- V1004 [CWE-628] The 'x32ByteHashedMessage' pointer was used unsafely after it was verified against nullptr. Check lines: 275, 281. iot_pkcs11.c 281
Анализатор предупреждает, что параметры функции, являющиеся указателями, используются небезопасно после того, как они были проверены на NULL. Действительно, аргументы проверяются, но в случае, если какой-нибудь из них оказывается равным NULL, никаких действий, кроме записи в xResult, не предпринимается. Этот участок кода как бы говорит: «Ага, значит, аргументы оказались плохими. Мы это сейчас запишем, а вы пока продолжайте, продолжайте».
Итог: в memcpy будет передан NULL. Что из этого может получиться? Куда будут скопированы значения и какие? На самом деле, гадать об этом не стоит, т.к. стандарт четко оговаривает, что такой вызов приводит к неопределенному поведению (смотри пункт 1).
В отчете анализатора еще остались примеры некорректной работы с указателями, которые я нашел в Amazon FreeRTOS, но я думаю, что приведённых примеров уже достаточно, чтобы показать вам возможности PVS-Studio в обнаружении подобных ошибок. Рассмотрим что-нибудь новенькое.
TRUE!= 1
Несколько ошибок, которые я нашел, были связаны с одним паттерном, про который, к сожалению, достаточно часто забывают.
Дело в том, что тип bool (из C++) отличается от типа BOOL (обычно используемого в C). Первый может содержать только значение true или false. Второй же является typedef’ом какого-нибудь целочисленного типа (int, long, и т.д). Для него «ложью» является значение 0, а «истиной» — любое значение, отличное от нуля.
Так как встроенный булев тип в языке Си отсутствует, то для удобства определяют вот такие константы:
#define FALSE 0
#define TRUE 1
Теперь рассмотрим пример:
int mbedtls_hardware_poll(void* data,
unsigned char* output,
size_t len,
size_t* olen)
{
int lStatus = MBEDTLS_ERR_ENTROPY_SOURCE_FAILED;
HCRYPTPROV hProv = 0;
/* Unferenced parameter. */
(void)data;
/*
* This is port-specific for the Windows simulator,
* so just use Crypto API.
*/
if (TRUE == CryptAcquireContextA(
&hProv, NULL, NULL,
PROV_RSA_FULL,
CRYPT_VERIFYCONTEXT))
{
if (TRUE == CryptGenRandom(hProv, len, output))
{
lStatus = 0;
*olen = len;
}
CryptReleaseContext(hProv, 0);
}
return lStatus;
}
Предупреждения PVS-Studio:
- V676 [CWE-253] It is incorrect to compare the variable of BOOL type with TRUE. aws_entropy_hardware_poll.c 48
- V676 [CWE-253] It is incorrect to compare the variable of BOOL type with TRUE. Correct expression is: 'FALSE!= CryptGenRandom (hProv, len, output)'. aws_entropy_hardware_poll.c 51
Нашли ошибку? А она есть :) Функции CryptAcquireContextA и CryptGenRandom — это стандартные функции из заголовка wincrypt.h. В случае успеха они возвращают ненулевое значение. Я подчеркну — ненулевое. Значит, теоретически это может быть любое значение, отличное от нуля: 1, 314, 42, 420.
Судя по всему, программист, который писал функцию из примера, не подумал об этом, и в итоге полученные значения сравниваются с единицей.
С какой вероятностью условие TRUE == CryptGenRandom (…) не выполнится? Сложно сказать. Может быть, CryptGenRandom и возвращает единицу чаще, чем другие значения, а может быть всегда возвращает только единицу. Мы не можем знать этого наверняка: реализация данной криптографической функции скрыта от глаз смертных программистов :)
Важно помнить, что подобные сравнения потенциально опасны. И вместо:
if (TRUE == GetBOOL())
использовать более безопасный вариант:
if (FALSE != GetBOOL())Проблемы с оптимизацией
Несколько предупреждений анализатора были связаны с медленно работающими конструкциями. Например:
int _IotHttpsDemo_GetS3ObjectFileSize(....)
{
....
pFileSizeStr = strstr(contentRangeValStr, "/");
....
}
Предупреждение PVS-Studio: V817 It is more efficient to seek '/' character rather than a string. iot_demo_https_common.c 205
Кратенько и понятно, не правда ли? Функция strstr здесь используется для поиска всего лишь одного символа, который передается в параметр как строка (он заключен в двойные кавычки).
Это место можно потенциально оптимизировать, заменив strstr на strchr:
int _IotHttpsDemo_GetS3ObjectFileSize(....)
{
....
pFileSizeStr = strchr(contentRangeValStr, '/');
....
}
Тогда поиск будет работать чуточку быстрее. Мелочь, а приятно.
Такие оптимизации — это, конечно, хорошо, и анализатор нашел еще одно место, которое можно оптимизировать гораздо заметнее:
void vRunOTAUpdateDemo(void)
{
....
for (; ; )
{
....
xConnectInfo.cleanSession = true;
xConnectInfo.clientIdentifierLength
= (uint16_t)strlen(clientcredentialIOT_THING_NAME);
xConnectInfo.pClientIdentifier
= clientcredentialIOT_THING_NAME;
....
}
}
Предупреждение PVS-Studio: V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. aws_iot_ota_update_demo.c 235
Хммм… Внутри цикла при каждой итерации вызывается strlen, который каждый раз вычисляет длину одной и той же строки. Не самая эффективная операция :)
Давайте заглянем в определение clientcredentialIOT_THING_NAME:
/*
* @brief Host name.
*
* @todo Set this to the unique name of your IoT Thing.
*/
#define clientcredentialIOT_THING_NAME ""
Пользователю предлагается ввести сюда имя своего устройства. По умолчанию оно пустое, и в этом случае всё хорошо. А что, если пользователь захочет ввести туда какое-нибудь длинное и красивое имя? Например, я бы с удовольствием назвал своё детище »The Passionate And Sophisticated Coffee Machine BarBarista-N061E The Ultimate Edition». Представляете, каково было бы моё удивление, если бы после этого моя прекрасная кофе-машина стала работать чуточку медленнее? Непорядок!
Чтобы исправить ошибку, стоит вынести strlen за тело цикла. Ведь имя устройства не меняется во время работы программы. Эххх, сюда бы constexpr из C++…
Хорошо, хорошо, признаю: здесь я немного сгустил краски. Как отметил мой коллега Андрей Карпов, современные компиляторы знают, что такое strlen и он лично наблюдал, как они в двоичном коде просто используют константу, если понимают, что длина строки не может измениться. Так что есть большая вероятность, что в режиме сборки релизной версии вместо настоящего расчёта длины строки будет просто использовано заранее вычисленное значение. Тем не менее, это работает не всегда, поэтому писать такой код — не очень хорошая практика.
Несколько слов про MISRA
Анализатор PVS-Studio имеет большой набор правил, позволяющих проверить ваш код на соответствие стандартам MISRA C и MISRA C++. Что это за стандарты такие?
MISRA — это стандарт кодирования для высокоответственных встраиваемых систем. Он содержит набор строгих правил и рекомендаций по написанию кода и налаживанию процесса разработки. Правил этих довольно много, и нацелены они не только на устранение серьезных ошибок, но и различных «code smells», а также на написание максимального понятного и читаемого кода.
Таким образом, следование стандарту MISRA не только помогает избежать ошибок и уязвимостей, но и значительно — значительно! — снизить вероятность их появления в уже существующем коде.
MISRA используется в аэрокосмической, медицинской, автомобильной и военной индустриях — там, где от качества встраиваемого ПО зависят человеческие жизни.
Судя по всему, разработчики Amazon FreeRTOS знают про этот стандарт, и по большей части следуют ему. Это правильно: если вы пишете ОС широкого профиля для встраиваемых систем, то вы обязаны думать о безопасности.
Однако я нашел достаточно много нарушений стандарта MISRA. Я не буду приводить здесь примеры правил вроде «не используйте union» или «функция должна иметь только один return в конце тела» — к сожалению, они не зрелищны, как и большинство правил MISRA. Лучше я приведу вам примеры нарушений, которые потенциально могут привести к серьезным последствиям.
Начнем с макросов:
#define FreeRTOS_ms_to_tick(ms) ( ( ms * configTICK_RATE_HZ + 500 ) / 1000 )#define SOCKETS_htonl( ulIn ) ( ( uint32_t ) \
( ( ( ulIn & 0xFF ) << 24 ) | ( ( ulIn & 0xFF00 ) << 8 ) \
| ( ( ulIn & 0xFF0000 ) >> 8 ) | ( ( ulIn & 0xFF000000 ) >> 24 ) ) )#define LEFT_ROTATE( x, c ) ( ( x << c ) | ( x >> ( 32 - c ) ) )
Предупреждения PVS-Studio:
- V2546 [MISRA C 20.7] The macro and its parameters should be enclosed in parentheses. Consider inspecting the 'ms' parameter of the 'FreeRTOS_ms_to_tick' macro. FreeRTOS_IP.h 201
- V2546 [MISRA C 20.7] The macro and its parameters should be enclosed in parentheses. Consider inspecting the 'ulIn' parameter of the 'SOCKETS_htonl' macro. iot_secure_sockets.h 512
- V2546 [MISRA C 20.7] The macro and its parameters should be enclosed in parentheses. Consider inspecting parameters 'x', 'c' of the 'LEFT_ROTATE' macro. iot_device_metrics.c 90
Да, это именно то, о чем вы подумали. Параметры этих макросов не обёрнуты в скобки. Если кто-то случайно напишет что-то вроде
val = LEFT_ROTATE(A[i] | 1, B);
то такой «вызов» макроса раскроется в:
val = ( ( A[i] | 1 << B ) | ( A[i] | 1 >> ( 32 - B ) ) );
Помните приоритеты операций? Сначала выполняется побитовый сдвиг, и только после него — побитовое «или». Поэтому логика программы будет нарушена. Более простой пример: что будет, если в макрос FreeRTOS_ms_to_tick передать выражение »x + y»? Одной из основных целей MISRA и является профилактика возникновения таких ситуаций.
Кто-то может возразить: «если у вас работают программисты, не знающие про такое, то вас уже никакой стандарт не спасёт!», и я с этим не соглашусь. Программисты — тоже люди, и каким бы опытным ни был человек, он тоже может устать и допустить ошибку под конец рабочего дня. Это одна из причин, по которой MISRA настоятельно рекомендует использовать автоматические средства анализа для проверки проекта на соответствие стандарту.
Обращусь к разработчикам Amazon FreeRTOS: PVS-Studio нашел еще 12 небезопасных макросов, так что вы там поаккуратнее с ними :)
Еще одно интересное нарушение MISRA:
/**
* @brief Callback for an asynchronous request to notify
* that the response is complete.
*
* @param[in] 0pPrivData - User private data configured
* with the HTTPS Client library request configuration.
* @param[in] respHandle - Identifier for the current response finished.
* @param[in] rc - Return code from the HTTPS Client Library
* signaling a possible error.
* @param[in] status - The HTTP response status.
*/
static void _responseCompleteCallback(void* pPrivData,
IotHttpsResponseHandle_t respHandle,
IotHttpsReturnCode_t rc,
uint16_t status)
{
bool* pUploadSuccess = (bool*)pPrivData;
/* When the remote server response with 200 OK,
the file was successfully uploaded. */
if (status == IOT_HTTPS_STATUS_OK)
{
*pUploadSuccess = true;
}
else
{
*pUploadSuccess = false;
}
/* Post to the semaphore that the upload is finished. */
IotSemaphore_Post(&(_uploadFinishedSem));
}
Сможете найти ошибку самостоятельно?
Предупреждение PVS-Studio: V2537 [MISRA C 2.7] Functions should not have unused parameters. Consider inspecting the parameter: 'rc'. iot_demo_https_s3_upload_async.c 234
Присмотритесь: нигде в теле функции не используется параметр rc. Причем в комментарии к функции явно написано, что этот параметр представляет собой код возврата другой функции, и что он может сигнализировать об ошибке. Тогда почему этот параметр никак не обрабатывается? Здесь явно что-то не то.
Впрочем, и без подобных комментариев неиспользуемые параметры часто указывают на нарушенную логику программы. Иначе зачем они нужны в сигнатуре функции?
Здесь я привел небольшую функцию, которая хорошо подходит для примера в статье. Помимо неё я нашел еще 10 неиспользуемых параметров. Многие из них используются в функциях побольше, и обнаружить их — не самое простое дело.
Подозрительно, что их не обнаружили раньше. Ведь компиляторы легко выявляют подобные случаи.
Заключение
Это были не все найденные анализатором проблемные места, но статья и так уже получилась довольно большой. Надеюсь, что благодаря ей разработчики Amazon FreeRTOS смогут исправить некоторые недочёты, а возможно даже захотят самостоятельно попробовать PVS-Studio. Так можно будет тщательнее изучить предупреждения, да и вообще — работать с удобным интерфейсом гораздо проще, чем разглядывать текстовый отчёт.
Спасибо за то, что читаете наши статьи! Увидимся в следующем выпуске: D
P.S. Так уж вышло, что эта статья вышла 31 октября. Поэтому желаю всем счастливого Хэллоуина!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov. On request of Embedded Developers: Detecting Errors in Amazon FreeRTOS.
