# Сканер сетевого трафика Snort под сканером анализатора кода PVS\-Studio Snort — это самая используемая система обнаружения вторжений \(IDS\) в мире\. Каждый, кто имеет дело с защитой информации, должен быть с ней знаком\. Имеет ли такой крутой инструмент ошибки или потенциальные уязвимости? Давайте попробуем в этом разобраться с помощью статического анализатора PVS\-Studio\. ![0812_Snort_ru/image1.png](https://import.viva64.com/docx/blog/0812_Snort_ru/image1.png) ## Введение IDS – система обнаружения вторжения, предназначенная для регистрации подозрительных действий в сети, таких как сетевые атаки против уязвимых сервисов; неавторизированный доступ к важным файлам; атаки, направленные на повышение привилегий; действия различных компьютерных вирусов, троянов и червей\. IDS обеспечивают дополнительный уровень защиты компьютерных систем\. _Snort_ — наиболее популярная свободная сетевая система предотвращения вторжений \(IPS\) и обнаружения вторжений \(IDS\)\. У неё открытый исходный код\. Snort способна выполнять регистрацию пакетов и в реальном времени осуществлять анализ трафика в IP\-сетях, блокировать и предотвращать атаки\. Она была создана Мартином Рошем \(Martin Roesch\) в 1999 году и стала настолько популярной, что сетевой гигант _Cisco_ приобрел её в 2014 году\. На данный момент есть две последние версии _Snort_\. Это _Snort_ 2\.9\.17 на языке Си и _Snort_ 3\.1\.1 на С\+\+\. В текущей статье мы проанализируем уже давно используемый _Snort_ на Cи, а в следующей – свежий Snort на С\+\+\. Подведем итоги и узнаем, какой код является более качественным\. ## PVS\-Studio [_PVS\-Studio_](https://pvs-studio.ru/ru/) – это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C\+\+, C\# и Java\. Работает в 64\-битных системах на Windows, Linux и macOS и может анализировать код, предназначенный для 32\-битных, 64\-битных и встраиваемых ARM платформ\. _PVS\-Studio_ наиболее эффективно использовать сразу после компиляции\. Это позволяет находить ошибки в коде до его тестирования и тратить меньше времени на отладку программы\. _Snort_ версии 2\.9\.17 написан на языке Cи и предназначен для Linux, поэтому будем использовать _PVS\-Studio_ для Linux\. Получить инструкцию по установке и запуску анализатора можем [здесь](https://pvs-studio.ru/ru/docs/manual/0039/) и [здесь](https://pvs-studio.ru/ru/docs/manual/0036/)\. ## Формирование отчёта с результатами анализа Проект Snort собирается с помощью команды _make_\. После просмотра [краткой инструкции](https://pvs-studio.ru/ru/docs/manual/0036/#ID17C4A439C4) достаточно просто найти нужную последовательность команд для проверки такого проекта\. В инструкции говорится, что для проверки нам понадобится утилита _strace_\. Итак, что нам нужно сделать? 1\) Запустим сборку snort с помощью команды ```cpp pvs-studio-analyzer trace – make ``` 2\) После успешной сборки начнем анализ с помощью команды: ```cpp pvs-studio-analyzer analyze -l path_to_PVS_Studio.lic \ -a GA;OP -o logfile.log -j ``` где * _path\_to\_PVS\_Studio\.lic_ – пусть к лицензии PVS\-Studio \(запросить триальный ключ для пробного использования можно [тут](https://pvs-studio.ru/ru/pvs-studio/download/)\); * _logfile\.log_ – файл с полным закодированным результатом анализа; * __ – число процессоров, которые мы отдадим для анализа; * \-_a GA;OP_ – группы диагностик, используемые для анализа \(по умолчанию применяется только_ GA_\)\. Ниже приведен список всех доступных сейчас и в ближайшем будущем групп диагностик: * GA – General analysis; * 64 – 64\-bit Analysis; * OP \- Micro\-optimizations; * CS \- Customers Specific Requests; * MISRA – MISRA guidelines; * AUTOSAR – AUTOSAR guidelines \(ожидается\); * OWASP – OWASP guidelines \(ожидается\)\. 3\) Теперь осталось только конвертировать результат анализа в удобный формат для ознакомления\. Здесь нам понадобится утилита [_Plog Converter_](https://github.com/viva64/plog-converter)_\._ Предлагаю воспользоваться [_FullHtml_](https://pvs-studio.ru/ru/docs/manual/0036/#ID235C479A35) отчётом в качестве формата вывода\. Он довольно удобен тем, что его можно просматривать на любом устройстве\. Возможна сортировка предупреждений по уровням, номерам диагностик, по группам и файлам\. Также можно в один клик перейти в любой файл с предупреждением, попав на нужную строку\. Если же кликнуть на номер диагностики, то произойдёт переход на страницу с её подробным описанием\. Другие способы изучения результатов анализа на Linux можно посмотреть [здесь](https://pvs-studio.ru/ru/docs/manual/0036/#ID14E9A2B2CD)\. При необходимости можно отфильтровать предупреждения как по группам, так и по конкретным номерам диагностик\. Чтобы сгенерировать _FullHtml_ отчёт для всех предупреждений _General analysis_ уровня _High_ и _Medium_, выполним следующую команду: ```cpp plog-converter -a GA:1,2 -t fullhtml logfile.log \ -o path_to_report_dir ``` где * _GA:1,2_ – набор диагностик общего назначения уровня _High_ и _Medium,_ * _path\_to\_project_ – путь к папке, в которую будет сгенерирован отчёт\. В этой статье я рассмотрел только предупреждения _General analysis_, так как их оказалось уже довольно много\. Для того чтобы сгенерировать отчёт с предупреждениями _Micro\-optimizations_, можно выполнить команду: ```cpp plog-converter -a OP:1,2,3 -t fullhtml path_to_project.log \ -o path_to_report_dir ``` Вернёмся к первому отчёту\. Откроем его в любом браузере и приступим к проверке\. ## Результаты проверки **Предупреждение N1 – И да, и нет равно нет** V560 A part of conditional expression is always false: \!p\-\>tcph\. sp\_rpc\_check\.c 285 V560 A part of conditional expression is always false: \!p\-\>udph\. sp\_rpc\_check\.c 286 ```cpp #define IsTCP(p) (IsIP(p) && p->tcph) #define IsUDP(p) (IsIP(p) && p->udph) int CheckRpc(void *option_data, Packet *p) { .... if (!p->iph_api || (IsTCP(p) && !p->tcph) || (IsUDP(p) && !p->udph)) { return 0; /* if error occured while ip header * was processed, return 0 automagically. */ } .... } ``` С виду вполне логичное условие после раскрытия макроса теряет смысл\. _PVS\-Studio_ сообщает нам, что выражение _\!p\-\>tcph_ всегда ложно, но почему? Дело в том, что если условие внутри макроса выполнилось, то _p\-\>tcph_ уже точно не равен нулю\. Раскрыв макрос, мы получим: ```cpp ((IsIP(p) && p->tcph) && !p->tcph) ``` Данное выражение всегда будет принимать значение _false_, так как _x && \!x \= 0_\. Точно такая же ситуация и со строкой кода ниже: ```cpp ((IsIP(p) && p->tcph) && !p->ucph) ``` Наверное, это совсем не то, что хотел автор\. Иначе бы он оставил только условие _if \(\!p\-\>iph\_api\)_\. Функция не проверяет, является ли переменная _p_ TCP или UDP, и поэтому может работать не совсем корректно\. **Предупреждение N2 – Опасный макрос** V634 The priority of the '\*' operation is higher than that of the '<<' operation\. It's possible that parentheses should be used in the expression\. bug34427\.c 160 ```cpp #define PM_EXP2(A) 1 << A int process_val(const u_int8_t *data, u_int32_t data_len, u_int32_t *retvalue, ....) { *retvalue = 0; .... /* Now find the actual value */ for (; i < data_len; i++) { *retvalue += data[i] * PM_EXP2(8 * (data_len - i - 1)); } return(0); } ``` Анализатор предупреждает, что после подстановки макроса результирующее выражение может вычисляться некорректно\. Сначала будет выполнено умножение на 1, а уже затем побитовый сдвиг на выражение в скобках\. Повезло, что в данной строке выражение _x \* 1 << y_ эквивалентно _x \* \(1 << y\)_\. Если слева или справа от макроса будет стоять _/_,_ %_,_ \+_,_ \-_ или прочие операции с приоритетом большим, чем у _<<,_ либо внутри макроса будет стоять операция с приоритетом меньшим, чем у _<<_, выражение будет вычисляться неправильно\. Всегда следует оборачивать макрос и его аргументы скобками, чтобы избежать проблем в будущем\. Так, правильным будет: ```cpp Define PM_EXP2(A) (1 << (A)) ``` Этот же опасный макрос удачно используется и в файле _misc\_ber\.c_ \(строка 97\)\. **Предупреждение N3 – Нечистоплотный компилятор** V597 The compiler could delete the 'memset' function call, which is used to flush 'ThisFmt' object\. The memset\_s\(\) function should be used to erase the private data\. ftpp\_ui\_config\.c 251 ```cpp void ftpp_ui_config_reset_ftp_cmd_format(FTP_PARAM_FMT *ThisFmt) { .... memset(ThisFmt, 0, sizeof(FTP_PARAM_FMT)); free(ThisFmt); } ``` Одной из важных задач компилятора является оптимизация\. Зачем записывать что\-то туда, где оно не будет использоваться? Функция _memset_ будет удалена и, возможно, приватные данные останутся не затертыми\. Анализатор предлагает использовать _memset\_s_, чтобы все работало так, как вы и задумали\. Эту функцию компилятор не тронет\. Как безопасно очищать приватные данные, можно посмотреть [здесь](https://pvs-studio.ru/ru/blog/posts/cpp/0388/)\. Ещё одна небезопасная очистка есть тут: _spo\_log\_tcpdump\.c_ 485 **Предупреждение** **N4** **–** **Неопределенность** V595 The 'ssd' pointer was utilized before it was verified against nullptr\. Check lines: 900, 910\. dce2\_smb2\.c 900 ```cpp void DCE2_Smb2Process(DCE2_SmbSsnData *ssd) { const SFSnortPacket *p = ssd->sd.wire_pkt; .... if (ssd && ssd->pdu_state != DCE2_SMB_PDU_STATE__RAW_DATA) { .... } .... } ``` Довольно странное поведение\. Сначала автор точно уверен, что указатель не нулевой, а потом засомневался в этом и проверяет его перед использованием\. При этом нигде между этими двумя строчками кода _ssd_ не использовался\. Чтобы не вводить других разработчиков и себя в том числе в заблуждение, было бы правильным либо добавить проверку везде, либо не проверять _ssd_ вовсе\. Другое предупреждение этой диагностики: V595 The 'it' pointer was utilized before it was verified against nullptr\. Check lines: 158, 160\. u2spewfoo\.c 158 ```cpp static inline void free_iterator(u2iterator *it) { if(it->file) fclose(it->file); if(it->filename) free(it->filename); if(it) free(it); } ``` Анализатор снова заметил странное поведение\. Здесь также уверенность в том, что указатель на что\-то указывает, потерялась по мере выполнения кода\. Проверку _it_ на _nullptr_ следовало бы сделать в самом начале\. Проблема разыменовывания нулевого указателя популярна среди разработчиков на С\\С\+\+\. Так и в этом проекте нашлось ещё 15 подобных предупреждений\. Там тоже не все однозначно\. Вот половина из них: * The 'bm\_variable\_name' pointer was utilized before it was verified against nullptr\. Check lines: 113, 128\. sf\_snort\_plugin\_byte\.c 113 * V595 The 'cursor' pointer was utilized before it was verified against nullptr\. Check lines: 293, 302\. sf\_snort\_plugin\_pcre\.c 293 * V595 The 'configNext' pointer was utilized before it was verified against nullptr\. Check lines: 782, 788\. spp\_imap\.c 782 * V595 The 'sub\-\>entries' pointer was utilized before it was verified against nullptr\. Check lines: 193, 197\. sfrt\_dir\.c 193 * V595 The 'sub\-\>lengths' pointer was utilized before it was verified against nullptr\. Check lines: 191, 207\. sfrt\_dir\.c 191 * The 'configNext' pointer was utilized before it was verified against nullptr\. Check lines: 778, 784\. spp\_pop\.c 778 * V595 The 'configNext' pointer was utilized before it was verified against nullptr\. Check lines: 809, 816\. spp\_smtp\.c 809 * V595 The 'pmd' pointer was utilized before it was verified against nullptr\. Check lines: 1754, 1761\. fpcreate\.c 1754 **Предупреждение N5 – Очистить пустоту** V575 The null pointer is passed into 'free' function\. Inspect the first argument\. sdf\_us\_ssn\.c 202 ```cpp int ParseSSNGroups(....) { FILE *ssn_file; char *contents; .... contents = (char *)malloc(length + 1); if (contents == NULL) { _dpd.logMsg("Sensitive Data preprocessor: Failed to allocate memory " "for SSN groups.\n"); fclose(ssn_file); free(contents); // <= return -1; } .... free(contents); return 0; } ``` В таком контексте функция _free_ всегда будет вызываться с аргументом ноль, а значит, она ничего не будет делать\. Компилятор исключит это действие во время оптимизации\. Возможно, функции _free_ тут просто не должно быть, а возможно, следовало бы освободить какой\-то другой участок памяти\. **Предупреждение N6 – Не поделили место** V519 The 'port\_array\[5061 / 8\]' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 327, 328\. sip\_config\.c 328 ```cpp #define PORT_INDEX(port) port / 8 #define SIP_PORT 5060 #define SIPS_PORT 5061 static void SIP_ParsePortList(char **ptr, uint8_t *port_array) { .... /* If the user specified ports, remove SIP_PORT for now since * it now needs to be set explicitly. */ port_array[PORT_INDEX(SIP_PORT)] = 0; port_array[PORT_INDEX(SIPS_PORT)] = 0; .... } ``` Анализатор предупреждает, что в одну и ту же ячейку памяти записывают значение дважды\. Это повод задуматься и проверить\. При раскрытии макросов становится ясно, что два разных порта имеют одну ячейку памяти\. Автору стоит обратить внимание на этот участок кода\. Может, здесь нужно использовать другой макрос или убрать одно из обнулений\. **Предупреждение N7 – Не на своем месте** V713 The pointer 'fileEntry\-\>context' was utilized in the logical expression before it was verified against nullptr in the same logical expression\. file\_segment\_process\.c 393 ```cpp static inline int _process_one_file_segment(void* p, FileEntry *fileEntry, ....) { .... if ((fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH) && fileEntry->context && fileEntry->context->sha256) { free(fileEntry->context->sha256); fileEntry->context->sha256 = NULL; } .... } ``` В одном и том же условном выражении сначала разыменовывается указатель, а затем проверяется на _nullptr_\. Это серьезная опечатка, которая приведёт работу программы к краху\. Наверное, уставший разработчик по невнимательности вставил дополнительное условие в самое начало выражения, а не в середину или конец\. Правильно было бы его записать так: ```cpp if ( fileEntry->context && fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH && fileEntry->context->sha256) ``` Другой возможный правильный вариант: ```cpp if ((fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH) && fileEntry->context->something && fileEntry->context->sha256 ``` Программа не устаёт\. Статический анализатор всегда просматривает каждый участок кода с одинаковой внимательностью и предупреждает об ошибках или странностях в коде\. Попробуйте [_PVS\-Studio_](https://pvs-studio.ru/ru/pvs-studio/download/) сами и убедитесь в этом\. **Предупреждение N8 – Вечный двигатель** V654 The condition '\!done' of loop is always true\. log\.c 207 ```cpp void PrintNetData(....) { int done; /* flag */ .... /* initialization */ done = 0; .... /* loop thru the whole buffer */ while(!done) { .... } .... } ``` Кажется, где\-то должен был быть выход, но его нет\. Переменная _done_ не меняется внутри цикла, что приводит к бесконечному выполнению\. В коде приведены все места, где встречается эта переменная\. Нет ни указателей на неё, ни ссылок\. Попав в этот цикл, программа непременно зависнет\. **Предупреждение N9 – Дважды проверь\!** V501 There are identical sub\-expressions '\!info\-\>sip\.\_\_in6\_u\.\_\_u6\_addr32\[0\]' to the left and to the right of the '&&' operator\. pkt\_tracer\.c 160 V501 There are identical sub\-expressions '\!info\-\>dip\.\_\_in6\_u\.\_\_u6\_addr32\[0\]' to the left and to the right of the '&&' operator\. pkt\_tracer\.c 167 ```cpp static inline void debugParse(...., DebugSessionConstraints *info) { .... if (!info->sip.s6_addr32[0] && !info->sip.s6_addr32[0] && !info->sip.s6_addr16[4] && info->sip.s6_addr16[5] == 0xFFFF) { saf = AF_INET; } else saf = AF_INET6; if (!info->dip.s6_addr32[0] && !info->dip.s6_addr32[0] && !info->dip.s6_addr16[4] && info->dip.s6_addr16[5] == 0xFFFF) { daf = AF_INET; } else daf = AF_INET6; .... } ``` В одной и той же функции дважды встречается двойная проверка _\!info\-\>sip\.s6\_addr32\[0\]_\. Работать от этого функция лучше не станет, а вот упустить важное условие может\. Скорее всего, была допущена опечатка в одном условном выражении, которая была скопирована и во второе\. Правильным вариантом могло быть: ```cpp !info->sip.s6_addr32[0] && !info->sip.s6_addr32[1] ``` или ```cpp !info->sip.s6_addr32[0] && !info->sip.s6_addr16[0] ``` или что\-то ещё\. Разработчику стоит проверить этот код\. Функция может работать несовсем корректно\. Точно такой же фрагмент кода, с такими же предупреждениями был найден в файле _fw\_appid\.c_: * V501\. There are identical sub\-expressions '\!info\-\>sip\.\_\_in6\_u\.\_\_u6\_addr32\[0\]' to the left and to the right of the '&&' operator\. fw\_appid\.c 864 * V501 There are identical sub\-expressions '\!info\-\>dip\.\_\_in6\_u\.\_\_u6\_addr32\[0\]' to the left and to the right of the '&&' operator\. fw\_appid\.c 871 **Предупреждение N10 – Закрыт навсегда** V621 Consider inspecting the 'for' operator\. It's possible that the loop will be executed incorrectly or won't be executed at all\. snort\_stream\_tcp\.c 2316 V654 The condition 'i < 0' of loop is always false\. snort\_stream\_tcp\.c 2316 ```cpp #define DEFAULT_PORTS_SIZE 0 static void StreamParseTcpArgs(....) { int i; .... for (i = 0; i < DEFAULT_PORTS_SIZE; i++) { .... } .... } ``` Сразу две диагностики ругаются на этот фрагмент кода\. Макрос _DEFAULT\_PORTS\_SIZE_ в релизной версии программы раскрывается в ноль, поэтому данный цикл никогда не будет выполняться\. Возможно, тут должен был использоваться другой макрос или же этот цикл предназначался для отладки и теперь является бесполезным\. **Предупреждение N11 – Утечка памяти** В начале рассмотрим два макроса: _BNFA\_MALLOC_ и _BNFA\_FREE\._ Макрос _BNFA\_MALLOC_ раскрывается следующим образом: ```cpp #define BNFA_MALLOC(n,memory) bnfa_alloc(n,&(memory)) static void * bnfa_alloc( int n, int * m ) { void * p = calloc(1,n); if( p ) { if(m) { m[0] += n; } } return p; } ``` А макрос _BNFA\_FREE_ скрывает в себе: ```cpp #define BNFA_FREE(p,n,memory) bnfa_free(p,n,&(memory)) static void bnfa_free( void *p, int n, int * m ) { if( p ) { free(p); if(m) { m[0] -= n; } } } ``` Теперь посмотрим предупреждение _PVS\-Studio_: V773 The function was exited without releasing the 'pi' pointer\. A memory leak is possible\. bnfa\_search\.c 1168 ```cpp static int _bnfa_conv_list_to_csparse_array(bnfa_struct_t * bnfa) { bnfa_state_t * ps; /* transition list */ bnfa_state_t * pi; /* state indexes into ps */ bnfa_state_t ps_index = 0; unsigned nps; .... ps = BNFA_MALLOC(nps*sizeof(bnfa_state_t), bnfa->nextstate_memory); if (!ps) { return -1; } bnfa->bnfaTransList = ps; pi = BNFA_MALLOC(bnfa->bnfaNumStates*sizeof(bnfa_state_t), bnfa->nextstate_memory); // <= if (!pi) { return -1; } .... if (ps_index > nps) { return -1; // <= } .... BNFA_FREE(pi,bnfa->bnfaNumStates*sizeof(bnfa_state_t), bnfa->nextstate_memory); return 0; } ``` Есть два указателя: _ps_ и _pi_\. Анализатор ругается только на _pi_\. Почему же? Дело в том, что участок памяти, выделенный для _ps_, запомнил другой, внешний для функции указатель _bnfa\-\>bnfaTransList_\. В этой функции не производится очистка ни _bnfa\-\>bnfaTransList_, ни _ps_, а значит, эта память вероятней всего используется и очищается в другом месте программы\. Совсем иначе дело обстоит с указателем _pi_\. Память, занятая _pi_, очищается в конце функции с помощью функции _BNFA\_FREE_\. Однако этого не произойдёт, если выполнится условие _ps\_index \> nps_\. Тогда функция завершится без очистки\. Чтобы функция работала корректно, нужно продублировать функцию очистки _pi_ и в тело этого условного оператора\. Похожая ситуация встретилась и в другом месте: V773 The function was exited without releasing the 'ips\_port\_filter\_list' pointer\. A memory leak is possible\. parser\.c 1854 **Предупреждение N12 – Бессмысленная проверка** V547 Expression 'rval \!\= \- 6' is always true\. output\_base\.c 219 ```cpp #define OUTPUT_SUCCESS 0 #define OUTPUT_ERROR -1 #define OUTPUT_ERROR_EXISTS -6 static int register_module(....) { .... int rval; if ((rval = register_plugin(current_dm)) != OUTPUT_SUCCESS) { if (rval != OUTPUT_ERROR_EXISTS) // <= { fprintf(stderr, "%s: Failed to register OUTPUT plugin.\n", current_dm->name); } return OUTPUT_ERROR; } .... } ``` Заглянем в функцию _register\_plugin_: ```cpp static int register_plugin(const Output_Module_t *dm) { if (....) { .... return OUTPUT_ERROR; } .... return OUTPUT_SUCCESS; } ``` Анализатор видит, что _rval_ принимает результат функции, а функция может возвращать либо _0_, либо _\-1_\. Следовательно, _rval_ не может иметь значение _\-6_\. Условие _if \(rval \!\= OUTPUT\_ERROR\_EXISTS\)_ бессмысленно\. _rval_ гарантированно имеет значение _\-1_\. Разработчику стоит обратить внимание на этот код\. Возможно, нужно проверять другую переменную, или есть опечатка в функции _register\_plugin_\. Подобный случай был обнаружен ещё в одном месте: V547 Expression 'ret \=\= \- 2' is always false\. base\.c 344 ```cpp #define OUTPUT_SUCCESS 0 #define OUTPUT_ERROR -1 #define OUTPUT_ERROR_NOMEM -2 #define OUTPUT_ERROR_INVAL -5 int output_load(const char *directory) { .... ret = output_load_module(dirpath); if (ret == OUTPUT_SUCCESS) { DEBUG_WRAP(DebugMessage(DEBUG_INIT, "Found module %s\n", de->d_name);); } else if (ret == OUTPUT_ERROR_NOMEM) // <= { closedir(dirp); return OUTPUT_ERROR_NOMEM; } .... } ``` Функция _output\_load\_module_ возвращает одно из значений: _\-5_, _\-1_, _0_\. Это значит, что условие _ret \=\= \-2_ всегда будет ложным\. Следует проверить условие или функцию\. Возможно, есть опечатка\. На этом закончились предупреждения уровня _High_\. Он включает предупреждения с максимальным уровнем достоверности\. Они часто указывают на ошибки, требующие немедленного исправления\. Уровень _Medium_ содержит менее достоверные предупреждения, на которые все же стоит обратить пристальное внимание\. Давайте посмотрим, что удалось обнаружить диагностикам уровня Medium\. **Предупреждение N13 – Макросная упаковка** V1004 The 'ppm\_pt' pointer wras used unsafely after it was verified against nullptr\. Check lines: 361, 362\. detect\.c 362 ```cpp ppm_pkt_timer_t *ppm_pt = NULL; int Preprocess(Packet * p) { .... if( PPM_PKTS_ENABLED() ) { PPM_GET_TIME(); PPM_TOTAL_PKT_TIME(); PPM_ACCUM_PKT_TIME(); .... } .... } #define PPM_TOTAL_PKT_TIME() \ if( ppm_pt) \ { \ ppm_pt->tot = \ ppm_cur_time - ppm_pt->start - ppm_pt->subtract; \ } #define PPM_ACCUM_PKT_TIME() \ snort_conf->ppm_cfg.tot_pkt_time += ppm_pt->tot; ``` Функция _Preprocess_ почти полностью состоит из макросов, в которые упакованы инструкции выполнения программы\. Это очень усложняет _читабельность_ кода\. Вероятность запутаться, что\-то недоглядеть и сделать ошибку повышается\. Так и случилось\. Два макроса, выполняющие определенные действия стоят друг за другом\. Раскрывая их, можно обнаружить, что в первом случае _ppm\_pt_ проверяют на nullptr, а во втором – уже нет\. Это совсем нелогично\. Если _ppm\_pt_ будет нулевым, программа все равно упадёт\. **Предупреждение N14 – Отладочный код** V547 Expression 'found\_offset' is always true\. sf\_snort\_plugin\_pcre\.c 202 ```cpp static int pcre_test(...., int *found_offset) { .... *found_offset = -1; .... if (found_offset) { *found_offset = ovector[1]; DEBUG_WRAP(DebugMessage(DEBUG_PATTERN_MATCH, "Setting buffer and found_offset: %p %d\n", buf, found_offset);); } return matched; } ``` Проверка не имеет смысла\. Если по адресу указателя было записано значение, то указатель не нулевой\. А если он не нулевой, то значение будет перезаписано\. Скорее всего, строчка _\*found\_offset \= \-1_ лишняя и была добавлена при отладке программы\. Если _found\_offset_ будет являться нулевым указателем, то программа завершится аварийно\. В другом месте диагностика выдала следующее предупреждение: V547 Expression 'sipMsg\-\>status\_code \> 0' is always true\. sip\_dialog\.c 806 ```cpp int SIP_updateDialog(SIPMsg *sipMsg, SIP_DialogList *dList, SFSnortPacket *p ) { int ret; .... if (sipMsg->status_code == 0) { ret = SIP_processRequest(....); } else if (sipMsg->status_code > 0) { ret = SIP_processResponse(....); } else { ret = SIP_FAILURE; } .... } ``` Все бы хорошо, но _sipMsg\-\>status\_code_ имеет тип _uint16\_t_\. Если этот элемент структуры _SIPMsg_ не равен нулю, то он может быть только больше нуля\. Условие в первом _else_ избыточно, а тело второго оператора _else_ недостижимо\. Тут нет ошибки, есть только избыточный код\. Его следует избегать, чтобы в дальнейшем на его изучение и доработку не тратилось время разработчика\. Подобное предупреждение было найдено ещё в 32 местах\. **Предупреждение N15 – Избыточность или опечатка?** V560 A part of conditional expression is always true: hnode\. spp\_frag3\.c 4366 ```cpp static int Frag3Prune(FragTracker *not_me) { SFXHASH_NODE *hnode; .... while (....) { hnode = sfxhash_lru_node(f_cache); if (!hnode) { break; } if (hnode && hnode->data == not_me) // <= } .... } ``` Проверка на нулевой указатель _hnode_ тут не нужна\. Если _hnode_ будет нулевым, условие и так будет пропущено\. Может, допущена опечатка и нужно было проверить какое\-то поле объекта \*_hnode_? Подобное предупреждение было найдено ещё в 39 местах\. **Предупреждение N16 – Дублированное условие** V581 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 300, 308\. sf\_snort\_plugin\_pcre\.c 308 ```cpp static int pcreMatchInternal(...., const uint8_t **cursor) { const uint8_t *buffer_start; int pcre_offset; int pcre_found; .... if (pcre_found) { if (cursor) { *cursor = buffer_start + pcre_offset; } } if (pcre_found) return RULE_MATCH; .... } ``` Дважды проверяется одно и то же, при этом выполняются разные действия\. Подозрительный код\. Может, это результат рефакторинга, а может, опечатка в условии, которая приведёт к ошибке в логике программы\. **Предупреждение N17 – break или return?** V1001 The 'portsweep' variable is assigned but is not used by the end of the function\. spp\_sfportscan\.c 596 ```cpp static int PortscanAlertTcp(PS_PROTO *proto, ....) { .... int portsweep = 0; if (!proto) return -1; switch (proto->alerts) { case PS_ALERT_ONE_TO_ONE: .... break; case PS_ALERT_ONE_TO_ONE_DECOY: .... break; case PS_ALERT_PORTSWEEP: .... portsweep = 1; break; case PS_ALERT_DISTRIBUTED: .... break; case PS_ALERT_ONE_TO_ONE_FILTERED: .... break; case PS_ALERT_ONE_TO_ONE_DECOY_FILTERED: .... break; case PS_ALERT_PORTSWEEP_FILTERED: .... portsweep = 1; // <= return 0; case PS_ALERT_DISTRIBUTED_FILTERED: .... break; default: return 0; } .... } ``` Одна из веток оператора выбора присваивает значение переменной, а после – завершает функцию\. Это как\-то странно\. Просмотр других веток подсказывает, что нужно поменять _return_ на _break_ или убрать присваивание\. **Предупреждение N18 – Если ноль не равен нулю** V1048 The 'ret' variable was assigned the same value\. sf\_snort\_plugin\_loop\.c 142 V1048 The 'ret' variable was assigned the same value\. sf\_snort\_plugin\_loop\.c 148 ```cpp int LoopInfoInitialize(...., Rule *rule, LoopInfo *loopInfo) { int ret; /* Initialize the dynamic start, end, increment fields */ ret = DynamicElementInitialize(rule, loopInfo->start); if (ret) { return ret; } ret = DynamicElementInitialize(rule, loopInfo->end); if (ret) { return ret; } ret = DynamicElementInitialize(rule, loopInfo->increment); if (ret) { return ret; } .... } ``` Ниже приведено определение функции _DynamicElementInitialize\._ Стоит обратить внимание на возвращаемое значение\. ```cpp int DynamicElementInitialize(Rule *rule, DynamicElement *element) { void *memoryLocation; if (!rule->ruleData) { DynamicEngineFatalMessage("ByteExtract variable '%s' " "in rule [%d:%d] is used before it is defined.\n", element->refId, rule->info.genID, rule->info.sigID); } switch (element->dynamicType) { case DYNAMIC_TYPE_INT_REF: memoryLocation = sfghash_find((SFGHASH*)rule->ruleData, element->refId); if (memoryLocation) { element->data.dynamicInt = memoryLocation; } else { element->data.dynamicInt = NULL; DynamicEngineFatalMessage("ByteExtract variable '%s' " "in rule [%d:%d] is used before it is defined.\n", element->refId, rule->info.genID, rule->info.sigID); //return -1; } break; case DYNAMIC_TYPE_INT_STATIC: default: /* nothing to do, its static */ break; } return 0; // <= } ``` Функция _DynamicElementInitialize_ всегда возвращает _0_, поэтому проверять возвращаемое значение _ret_ в функции _LoopInfoInitialize_ нет никакого смысла\. Нет смысла вообще что\-то возвращать, если есть всего один вариант значения\. Ранее разработчики, возможно, экспериментировали со значением _\-1_ \(об этом свидетельствует закомментированный код\), но сейчас этот код не используется\. Подобное предупреждение встречалась ещё в 15 местах\. В результате проверки системы обнаружения вторжения _Snort_ c помощью статического анализатора _PVS\-Studio_ было обнаружено 35 потенциально опасных мест или ошибок, а также около 100 участков кода, которые требуют внимания разработчиков\. Есть вероятность, что они работают совсем не так, как было задумано\. Это не так много для 470 000 строк кода на языке Си\. Разработчики проекта _Snort_ хорошо выполнили свою работу\. Они сделали свой продукт максимально продуманным и с минимальным числом ошибок\. Использовав _PVS\-Studio_, они бы могли сделать код ещё более качественным и, скорее всего, сэкономить довольно большую часть времени, потраченную на отладку\. В следующей статье мы проанализируем _Snort_, написанный на C\+\+, и сравним результаты двух анализов\. Это позволит увидеть, какие паттерны ошибок больше характерны для Си, а какие – для C\+\+\. Также посмотрим, стал ли код более качественным или расширение функционала привело к большему числу ошибок\. ## Заключение _PVS\-Studio_ – удобный и полезный инструмент для разработчика\. Он приходит на помощь, когда в человеческий мозг перестают влезать многоуровневые зависимости в коде, когда программист теряет внимательность в результате усталости, когда модифицируются большие файлы и не все тонкости работы программы можно легко заметить, чтобы дописать код корректно\. Статический анализатор – это программа, которая будет покорно и всегда внимательно проверять код\. Используйте [_PVS\-Studio_](https://pvs-studio.ru/ru/pvs-studio/) во время разработки, и вы сэкономите часть своего времени и нервов\.