Георгий Грибков

31 Окт 2019

Теги:

#Cpp #OS #Embedded

По заказам Embedded-разработчиков: ищем ошибки в Amazon FreeRTOS

31 Окт 2019

Автор: Георгий Грибков

Немного о проектах
Как проводилась проверка
Что скрывает FreeRTOS
Что скрывает Amazon FreeRTOS
Несколько слов про MISRA
Заключение

Каждый, кто программирует микроконтроллеры, наверняка знает о 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 вернет 0x00010000, то при конвертации "единичка" потеряется, и условие выполнится.

На самом деле странно видеть такое приведение в условии. Зачем вообще его делать, если с нулём можно сравнить и обычный 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 октября. Счастливого Хэллоуина, ребята!

#Cpp #OS #Embedded