# Проверяем MatrixSSL с помощью PVS\-Studio и Cppcheck В статье я хочу рассказать о проверке проекта MatrixSSL статическими анализаторами C/C\+\+ PVS\-Studio и Cppcheck\. ![0304_MatrixSSL_PVS-Studio_ru/image1.png](https://import.viva64.com/docx/blog/0304_MatrixSSL_PVS-Studio_ru/image1.png) _Статья написана Павлом Пименовым, являющимся автором открытого peer\-to\-peer клиента [FlylinkDC\+\+](http://www.flylinkdc.ru/)\. Статья публикуется в нашем блоге с его разрешения\._ Порадовало наличие "из коробки" проекта для MS Visual Studio 2010\. Например, чтобы собрать openSSL из исходников в Visual C\+\+ нужно немного поприседать с бубном :\)\. Поэтому многие разработчики под windows используют готовые бинарные сборки openSSL, такие как [Win32 OpenSSL Installation Project](https://slproweb.com/products/Win32OpenSSL.html)\. [MatrixSSL](https://www.rambus.com/security/software-protocols/secure-communication-toolkits/tls-toolkit/) \- альтернативная библиотека алгоритмов шифрования распространяемым под лицензией GNU \(также возможно получение коммерческой поддержки\)\. Исходный код open source версии можно получить на официальном сайте\. Анализу подвергалась актуальная версия 3\.7\.1\. ## Об анализаторе * [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) \- коммерческий статический анализатор, выявляющий ошибки в исходном коде приложений на языке C/C\+\+/C\+\+11\. \(использовалась версия PVS\-Studio 5\.21\)\. * [Cppcheck](http://cppcheck.sourceforge.net/) \- бесплатный статический анализатор с открытым исходным кодом \(использовалась версия Cppcheck 1\.68\)\. ## Результаты проверки в PVS\-Studio ### Зачистка памяти [V512](https://pvs-studio.ru/ru/docs/warnings/v512/) A call of the 'memset' function will lead to underflow of the buffer 'ctx\-\>pad'\. hmac\.c 136, 222, 356 ```cpp ... // crypto\digest\digest.h typedef struct { #ifdef USE_SHA384 unsigned char pad[128]; #else unsigned char pad[64]; #endif int32 psHmacMd5Final(psHmacContext_t *ctx, unsigned char *hash) { memset(ctx->pad, 0x0, 64); return MD5_HASH_SIZE; } ... ``` По коду в этих трех функция все корректно и затирается только часть массива, которая используется, но анализатор подсказывает о возможно избыточном размере заказанного буфер размером 128 байт\. Думаю, здесь всё хорошо, но всё равно лучше для красоты затирать 64 или 128 байт\. Можно написать так: ```cpp memset(ctx->pad, 0x0, sizeof(ctx->pad)); ``` [V597](https://pvs-studio.ru/ru/docs/warnings/v597/) The compiler could delete the 'memset' function call, which is used to flush 'tmp' buffer\. The RtlSecureZeroMemory\(\) function should be used to erase the private data\. aes\.c 1139 ```cpp ... int32 psAesEncrypt(psCipherContext_t *ctx, unsigned char *pt, unsigned char *ct, uint32 len) { unsigned char tmp[MAXBLOCKSIZE]; ..... memset(tmp, 0x0, sizeof(tmp)); return len; } ... ``` Оптимизатор выкидывает вызов стандартной функции memset\(\)\. Для библиотеки шифрования вероятно это критично и является потенциальной дырой\. Аналогичные проблемные места: aes\.c 1139, aes\.c 1190, aes\.c 1191, des3\.c 1564, des3\.c 1609, des3\.c 1610, corelib\.c 304, pkcs\.c 1625, pkcs\.c 1680, pkcs\.c 1741 [V676](https://pvs-studio.ru/ru/docs/warnings/v676/) It is incorrect to compare the variable of BOOL type with TRUE\. Correct expression is: 'QueryPerformanceFrequency\(& hiresFreq\) \=\= FALSE'\. osdep\.c 52, 55 ```cpp ... #define PS_TRUE 1 #define PS_FALSE 0 int osdepTimeOpen(void) { if (QueryPerformanceFrequency(&hiresFreq) != PS_TRUE) { return PS_FAILURE; } if (QueryPerformanceCounter(&hiresStart) != PS_TRUE) { return PS_FAILURE; } ... ``` PS\_TRUE объявлена как "1"\. В MSDN про возврат функции QueryPerformanceFrequency написано "If the installed hardware supports a high\-resolution performance counter, the return value is nonzero" Т\.е\. надежнее написать QueryPerformanceCounter\(\) \=\= PS\_FALSE [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression '\(id \= ssl\-\>sessionId\) \=\= \(\(void \*\) 0\)' is always false\. Pointer 'id \= ssl\-\>sessionId' \!\= NULL\. matrixssl\.c 2061 ```cpp ... typedef struct ssl { ... unsigned char sessionIdLen; unsigned char sessionId[SSL_MAX_SESSION_ID_SIZE]; int32 matrixUpdateSession(ssl_t *ssl) { #ifndef USE_PKCS11_TLS_ALGS unsigned char *id; uint32 i; if (!(ssl->flags & SSL_FLAGS_SERVER)) { return PS_ARG_FAIL; } if ((id = ssl->sessionId) == NULL) { return PS_ARG_FAIL; } ... ``` Тут явная ошибка: условие никогда не выполнится так как sessionId объявлен как массив из 32 байт и у него не может быть адреса равного NULL\. Ошибка конечно не серьезная\. Пожалуй, это можно назвать и просто лишней бессмысленной проверкой\. [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: 0x00000002\. osdep\.c 265 ```cpp ... #define FILE_SHARE_READ 0x00000001 #define FILE_SHARE_WRITE 0x00000002 if ((hFile = CreateFileA(fileName, GENERIC_READ, FILE_SHARE_READ && FILE_SHARE_WRITE, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL)) == INVALID_HANDLE_VALUE) { psTraceStrCore("Unable to open %s\n", (char*)fileName); return PS_PLATFORM_FAIL; ... ``` Опечатка: вместо FILE\_SHARE\_READ \| FILE\_SHARE\_WRITE записали && получилось 1 && 2 \=\= 1 что эквивалентно одному FILE\_SHARE\_READ\. ### Возможно ошибочное условие [V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting the '\* c \!\= 0 && \* c \=\= 1' expression\. The expression is excessive or contains a misprint\. ssldecode\.c 3539 ```cpp ... if (*c != 0 && *c == 1) { #ifdef USE_ZLIB_COMPRESSION ssl->inflate.zalloc = NULL; ... ``` ### Возможная просадка производительности [V814](https://pvs-studio.ru/ru/docs/warnings/v814/) Decreased performance\. The 'strlen' function was called multiple times inside the body of a loop\. x509\.c 226 ```cpp ... memset(current, 0x0, sizeof(psList_t)); chFileBuf = (char*)fileBuf; while (fileBufLen > 0) { if (((start = strstr(chFileBuf, "-----BEGIN")) != NULL) && ... start += strlen("CERTIFICATE-----"); if (current == NULL) { ... ``` Тут анализатор внутри цикла while\(\) обнаружил вызов strlen\(\) для параметра который не меняется, в общем случае это не оптимально, но в данном конкретном т\.к\. на вход strlen\(\) передается константа известная на этапе компиляции, то оптимизатор в режиме /O2 вообще уберет вызов функции и подставит значение константы вычисленное на этапе компиляции\. ## Результаты проверки в Cppcheck Этот анализатор показал меньше предупреждений, но есть те которые PVS\-Studio не смог детектировать\. Все они на работу библиотеки не влияют так как лежат в юнит\-тестах относятся в crypto\\test\. ### "Контрольный return в голову" Consecutive return, break, continue, goto or throw statements are unnecessary\. The second statement can never be executed, and so should be removed\. ```cpp ... int32 psSha224Test(void) { runDigestTime(&ctx, HUGE_CHUNKS, SHA224_ALG); return PS_SUCCESS; return PS_SUCCESS; } ... ``` Ошибка copy\-paste\. В конце две одинаковых строчки: return PS\_SUCCESS;\. Аналогичная опечатка находится в функции psSha384Test\(void\)\. ### Утечка памяти Memory leak: table Неопасная в данном случае ошибка, но приятно что Cppcheck ее ловит код находится в файлах и выглядит так \(copy\-paste\): * crypto\\test\\eccperf\\eccperf\.c * crypto\\test\\rsaperf\\rsaperf\.c ```cpp ... table = malloc(tsize * sizeof(uint32)); if ((sfd = fopen("perfstat.txt", "w")) == NULL) { return PS_FAILURE; } ... ``` Ресурсы лучше заказывать перед тем когда они действительно необходимы\. Если посмотреть код в этих файлах, то окажется что table вообще не используется, т\.е\. тут вызов функции malloc\(\) и в конце функции free\(table\) лишние\. ## Заключение Я явлюсь разработчиком [FlylinkDC\+\+](http://www.flylinkdc.ru/) и уже более двух лет использую анализатор PVS\-Studio, который нам подарили как Open Source проекту\. Анализатор неоднократно помогал локализовать различные ошибки, как в своем коде, так и в коде сторонних библиотек\. Благодаря регулярным проверкам код FlylinkDC\+\+ стал немного надёжней и безопасней\. И это замечательно\.