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

    Каждый, кто программирует микроконтроллеры, наверняка знает о FreeRTOS, или по крайней мере слышал об этой операционной системе. Ребята из Amazon решили расширить возможности этой операционной системы для работы с сервисами AWS Internet of Things – так появилась Amazon FreeRTOS. Нас, разработчиков анализатора кода PVS-Studio, в почте и в комментариях под статьями попросили проверить эти проекты. Что ж, вы просили – мы сделали. Что из этого получилось – читайте далее.

    Рисунок 3

    Немного о проектах


    Для начала расскажу немного о «папе» проверяемого проекта – 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).

    Рисунок 2


    В отчете анализатора еще остались примеры некорректной работы с указателями, которые я нашел в 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 неиспользуемых параметров. Многие из них используются в функциях побольше, и обнаружить их – не самое простое дело.

    Подозрительно, что их не обнаружили раньше. Ведь компиляторы легко выявляют подобные случаи.

    Рисунок 1


    Заключение


    Это были не все найденные анализатором проблемные места, но статья и так уже получилась довольно большой. Надеюсь, что благодаря ей разработчики Amazon FreeRTOS смогут исправить некоторые недочёты, а возможно даже захотят самостоятельно попробовать PVS-Studio. Так можно будет тщательнее изучить предупреждения, да и вообще – работать с удобным интерфейсом гораздо проще, чем разглядывать текстовый отчёт.

    Спасибо за то, что читаете наши статьи! Увидимся в следующем выпуске :D

    P.S. Так уж вышло, что эта статья вышла 31 октября. Поэтому желаю всем счастливого Хэллоуина!



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov. On request of Embedded Developers: Detecting Errors in Amazon FreeRTOS.
    PVS-Studio
    784,56
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Комментарии 18

      +4

      Похоже в Amazon разработчики уже разучились нормально писать на Си.

        +3
        Если приводимые структуры имеют одинаковое выравнивание – значит, такое приведениене является ошибкой. И я не нашел ни одного опасного приведения!

        Тут пропала частица «не» или я тупой?
          0
          Тут пропал пробел перед этой частицей. Спасибо за внимательность!
          0
          del
            0
            Так как встроенный булев тип в языке Си отсутствует, то для удобства определяют вот такие константы:

            А как же тип _Bool?!
              0
              Ну или bool из stdbool, если делать более стильно, модно и молодёжно
                0
                #typedef _Bool bool…
              +2

              А не хотели бы вы посмотреть sel4? Отличаются от остальных тем что у них задекларировано формальное доказательство корректности кода. Теоретически кол-во ошибок там должно быть равно 0, а вот будет ли так же на практике?
              PS Буду с удовольствием читать все статьи о проверки эмбедед ос.

                –4
                Безотносительно к качеству — народ как-то не очень доволен FreeRTOS — forum.ixbt.com/topic.cgi?id=48:11736 шедулер много времени занимает, и прерывания блочатся при этом…
                  0
                  Безотносительно к вам — народ там просто не умеет ее готовить
                    0
                    Просто нужно правильно подготовить само железо для ее использования. Прерывания, которые вызывают процедуры FreeRTOS должны быть по приоритету ниже шедулера, а те, которые не вызывают, могут иметь более высокий приоритет и не будут блочиться при этом.
                    0

                    GGribkov, извините за оффтоп.


                    А может ли PVS-Studio помочь с оптимизацией заголовочных файлов? И, как следствие, со временем и качеством (пере)сборки.


                    1. Обнаружить недостающие
                      1.1 Безусловно — без них не проходит минимальная компиляция #include + main()
                      1.2 Условно 1 — без них проходит минимальная компиляция #include + main(), но, например, необъявленный класс используется в макросе или шаблоне
                      1.3 Условно 2 — без них не проходит минимальная компиляция #include + main(), но вместо #include можно обойтись предварительным объявлением (forward declaration)


                    2. Обнаружить лишние:
                      2.1 Безусловно — без них проходит минимальная компиляция #include + main()
                      2.2 Условно 1 — без них проходит минимальная компиляция #include + main(), но необъявленный класс используется в макросе или шаблоне
                      2.3 Условно 2 — без них не проходит минимальная компиляция #include + main(), но #include можно заменить предварительным объявлением (forward declaration)


                      +1
                      Такой функциональности нет.
                        0

                        А эта фунциональность кажется вам интересной? Полезной? Осуществимой?


                        • А может быть вам известны реализации подобного?
                          0
                          Эта функциональность интересная, но весьма сложна в реализации. Более того, она не будет точна. Например, в одном режиме компиляции h-файл может быть не нужен, но он нужен в другом режиме и удалять его нельзя.
                            0

                            Большинство (?) диагностик PVS-Studio не имеют 100% точности, так что это ещё не причина умывать руки :)


                            Удалять и не надо. Либо пользователь отметит это предупреждение как false positive, либо, если возможно, добавит вокруг что-то вроде #if[n]def _DEBUG


                            Когда-то, лет 15 назад, для проекта с ~1000 headers я написал скрипт, который нашел минимальный набор (в самом грубом приближении) перебором — просто удаляя и добавляя #include и пробуя компилировать с пустым main(). Тогда, емнип, время сборки уменьшилось на ~20%, и размеры на ~10%. С вашей базой можно добиться куда большего ;)


                            В общем, может эта идея и пригодится когда-нибудь.

                              0
                              размеры на ~10%
                              Я думал линковщик с компиляторов лишние include умеют прибивать, а тут вот как.
                                0
                                скорее всего какие-то следы дебажной информации (имена неиспользуемых перечислений, или функций) остались. Иначе — странно.

                    Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                    Самое читаемое