Давно хотелось написать статью, что юнит-тесты — это хорошо, но не стоит забывать, что они тоже могут содержать ошибки. Сейчас встретился проект DPDK, тесты которого хорошо демонстрируют этот нюанс. Давайте посмотрим, как выглядят типичные ошибки в юнит-тестах, и как они выявляются с помощью статического анализа кода.
Любой программист понимает, что юнит-тесты, создаваемые для выявления ошибок, сами могут содержать ошибки. Чаще всего эти ошибки приводят к тому, что тесты на самом деле не выполняют задуманные проверки.
В общем, про это знают, но эта тема не обсуждается. Злого умысла или специального замалчивания нет, но есть две причины почему так происходит.
Во-первых, ошибки в юнит-тестах не так уж критичны. То, что тест содержит ошибку, вовсе не означает, что ошибка есть в коде, который он проверяет. Неработающий тест можно сравнить с его отсутствием. Отсутствие теста — это плохо, но и не трагедия.
Я не говорю, что ошибки в юнит-тестах вообще не важны. Из-за этого в приложении может быть вовремя не выявлена настоящая ошибка. Однако согласитесь, очень низка вероятность, что именно из-за конкретного неработающего теста в приложении прячется страшная уязвимость. Скорее всего, найдя баг в тестах, программист просто его исправит и ничего не изменится. Про такие баги не будешь писать в статьях или рассказывать про них на конференциях.
Во-вторых, программистам не очень понятно, как выявлять такие ошибки. Пользователи приложения про них не пишут, так как вообще не сталкиваются с юнит-тестами. Ручное тестирование тоже мимо. Да и вообще не очень понятно, как именно тестировать юнит-тесты. Не будешь же писать юнит-тесты на юнит-тесты. А кто напишет юнит-тесты для юнит-тестов, тестирующих юнит-тесты? :)
Можно (и нужно) выполнять обзор кода тестов. Но это не панацея. Тем более, код тестов скучен, сложно сосредоточиться на его проверке. Можно немного попереживать про всё это, но повода для большой дискуссии нет.
Может вообще игнорировать проблему ошибок в юнит-тестах? На отдельные ошибки можно закрыть глаза, но в целом — нет, нужно их искать. Дело в том, что их там много. В свете того, что тесты не тестируются, баги живут и процветают в них. У них там гнездо :). Так что, всё-таки рационально что-то делать для уменьшения их количества.
В общем, понятно, почему тема непопулярная, хотя и хочется иметь поменьше ошибок в тестах. Как этого достичь?
Хорошими помощниками являются динамические и статические анализаторы кода.
Динамический анализ кода. Время от времени можно запускать юнит-тесты вместе с динамическим анализатором кода. Регулярно это делать, скорее всего, будет проблематично, так как в таком режиме тесты могут работать в десятки раз медленнее. Компромиссом может быть запуск в таком режиме по ночам или на выходных.
От таких запусков будет двойная польза. Во-первых, разработчики будут стараться писать юнит-тесты так, чтобы они использовали как можно больше кода приложения. Т.е. заглянули в как можно большее количество веток алгоритмов. Соответственно, большой процент кода будет исполнен под контролем динамического анализатора, что поможет выявить там ошибки. Во-вторых, анализатор проверит и сами юнит-тесты.
Впрочем, не всё так радужно. Большинство ошибок в юнит-тестах неуловимы для динамических анализаторов. Дальше из примеров ошибок будет видно, что они не нарушают что-то (не являются утечками памяти или выходом за границу массива). Тесты просто проверяют не всё, что должны.
Статический анализ кода. Статические анализаторы очень хорошо дополняют юнит-тесты. Например, они хорошо находят баги, которые прячутся в обработчиках ошибок и до которых тяжело добраться в юнит-тестах. Сложно написать тест, который симулирует обрыв связи при чтении файла по сети и проверит, что программа корректно отработает эту ситуацию.
Почему некоторые вещи тяжело/невозможно найти юнит-тестами я рассказывал в докладе "Как статический анализ дополняет TDD".
Статические анализаторы хорошо находят ошибки и в самих юнит-тестах. Они куда более внимательно изучают тесты, чем люди при обзоре кода. В отличии от людей такой код им не скучен и не утомляет :)
Мне давно хотелось продемонстрировать, как статический анализ помогает улучшить юнит-тесты. Я планировал сесть, собрать примеры из нескольких последних наших статей, но как-то всё руки не доходили. И вдруг подарок судьбы — проект DPDK, который мы проверили для написания очередной заметки.
Data Plane Development Kit (DPDK) — это набор открытых библиотек, способствующий ускорению обработки пакетов за счёт взаимодействия сетевого оборудования напрямую с приложениями, минуя ядро Linux.
В его юнит-тестах оказалось достаточно ошибок для написания статьи. Собственно эта статья перед вами. Посмотрите примеры багов в юнит-тестах и как ловко их можно искать с помощью PVS-Studio.
Примечание. Ошибки, найденные в коде DPDK, но не относящиеся к тестам, я рассмотрю в отдельной статье.
Не каждая функция содержит в своём названии test. Так почему я решил, что весь рассматриваемый далее код относится к тестам? Ответ прост: файлы с этим кодом располагаются в папках test*.
#define MAX_PKT_BURST (512)
static int
test_activebackup_rx_burst(void)
{
....
int i, j, burst_size = 17;
....
for (i = 0; i < test_params->bonding_member_count; i++) {
/* Generate test bursts of packets to transmit */
TEST_ASSERT_EQUAL(generate_test_burst(
&gen_pkt_burst[0], burst_size, 0, 1, 0, 0, 0),
burst_size, "burst generation failed");
....
/* free mbufs */
for (i = 0; i < MAX_PKT_BURST; i++) {
if (rx_pkt_burst[i] != NULL) {
rte_pktmbuf_free(rx_pkt_burst[i]);
rx_pkt_burst[i] = NULL;
}
}
/* reset bonding device stats */
rte_eth_stats_reset(test_params->bonding_port_id);
}
....
}Предупреждение PVS-Studio:
V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2239, 2291. test_link_bonding.c 2291
Два цикла используют одну и ту же переменную i в качестве счётчика:
for (i = 0; i < test_params->bonding_member_count; i++) {
....
for (i = 0; i < MAX_PKT_BURST; i++) {Если бы здесь возникал вечный цикл, то ошибка в тесте сразу была бы замечена и исправлена. Значит он не возникает. Почему?
Видимо, значение константы MAX_PKT_BURST (равное 512) всегда больше или равно значению переменной test_params->bonding_member_count. Внешний цикл сразу останавливается, выполнив только одну итерацию.
Таким образом, тест работает только чуть-чуть :)
Кстати, эта ошибка размножена копированием кода, и её можно встретить в этом же файле ниже: V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 4340, 4390. test_link_bonding.c 4390
static int
test_set_primary_member(void)
{
....
TEST_ASSERT_SUCCESS(memcmp(&read_mac_addr, &read_mac_addr,
sizeof(read_mac_addr)),
"bonding port mac address not set to that of primary port\n");
....
}Предупреждение PVS-Studio:
V549 The first argument of 'memcmp' function is equal to the second argument. test_link_bonding.c 795
Функция memcmp сравнивает сама с собой один и тот же буфер памяти. Естественно, функция всегда будет возвращать статус 0 (два блока памяти содержат идентичные данные). Явная опечатка из-за невнимательности или спешки.
Юнит-тест ничего не проверяет.
Видимо, одним из аргументов должен быть указатель expected_mac_addr. Я делаю такой вывод, так как неподалёку можно найти вот такой вызов memcmp:
TEST_ASSERT_SUCCESS(memcmp(expected_mac_addr, &read_mac_addr,
sizeof(read_mac_addr)),
"bonding port mac address not set to that of primary port\n");Вначале рассмотрим, как объявлена функция rte_ipv6_get_next_ext.
/**
* Parse next IPv6 header extension
* ....
* @return
* next protocol number if proto is an IPv6 extension, -EINVAL otherwise
*/
static inline int
rte_ipv6_get_next_ext(const uint8_t *p, int proto, size_t *ext_len);Обратите внимание, что функция в случае ошибки возвращает отрицательное значение (-EINVAL, а именно -22).
Теперь посмотрим, как эта функция используется в тестах.
test_vector_payload_populate(....)
{
....
int proto;
....
proto = hdr->proto;
p += sizeof(struct rte_ipv6_hdr);
while (proto != IPPROTO_FRAGMENT &&
(proto = rte_ipv6_get_next_ext(p, proto, &ext_len) >= 0)) // <=
p += ext_len;
/* Found fragment header, update the frag offset */
if (proto == IPPROTO_FRAGMENT) {
....
}Предупреждение PVS-Studio:
V593 Consider reviewing the expression of the 'A = B >= C' kind. The expression is calculated as following: 'A = (B >= C)'. test_security_inline_proto_vectors.h 523
Программист хотел:
Для этого он написал выражение:
(proto = rte_ipv6_get_next_ext(p, proto, &ext_len) >= 0)Но оно работает по-другому. Приоритет операции сравнения >= выше, чем у оператора присваивания =. В результате в переменную proto будет помещён результат проверки, то есть 0 или 1. Дальше юнит-тест работает с некорректными значением и проверяет не пойми что :).
Кажется программист знал про приоритет операций и использовал дополнительные скобки. Но опечатался и правую скобку поставил не туда, где нужно. Корректный вариант кода:
(proto = rte_ipv6_get_next_ext(p, proto, &ext_len)) >= 0static inline void
evdev_set_conf_values(struct rte_event_dev_config *dev_conf,
struct rte_event_dev_info *info)
{
memset(dev_conf, 0, sizeof(struct rte_event_dev_config));
dev_conf->dequeue_timeout_ns = info->min_dequeue_timeout_ns;
dev_conf->nb_event_ports = NB_TEST_PORTS;
dev_conf->nb_event_queues = NB_TEST_QUEUES;
dev_conf->nb_event_queue_flows = info->max_event_queue_flows;
dev_conf->nb_event_port_dequeue_depth = info->max_event_port_dequeue_depth;
dev_conf->nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth;
dev_conf->nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth;
dev_conf->nb_events_limit = info->max_num_events;
}Предупреждение PVS-Studio:
V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1166, 1168. test_event_crypto_adapter.c 1168
Анализатор обратил внимание на повтор:
dev_conf->nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth;
dev_conf->nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth;Давайте посмотрим на структуру rte_event_dev_config. Я подписал, каким полям присваиваются какие-то значения.
struct rte_event_dev_config {
uint32_t dequeue_timeout_ns; // Есть присваивание
int32_t nb_events_limit; // Есть присваивание
uint8_t nb_event_queues; // Есть присваивание
uint8_t nb_event_ports; // Есть присваивание
uint32_t nb_event_queue_flows; // Есть присваивание
uint32_t nb_event_port_dequeue_depth; // Есть присваивание
uint32_t nb_event_port_enqueue_depth; // Есть присваивание дважды
uint32_t event_dev_cfg; //
uint8_t nb_single_link_event_port_queues; //
};В начале вся структура заполняется нулями (см. вызов функции memset). Затем все поля, кроме двух последних, инициализируются новыми значениями. При этом полю nb_event_port_enqueue_depth значение присваивается дважды. Всё это очень подозрительно.
Возможно, вторая строчка с присваиванием просто лишняя. Возможно, забыли инициализировать какое-то другое поле. А возможно даже забыли инициализировать два поля. Как именно должно быть, я не знаю, так как не знаком с кодом проекта, но аномалия на лицо.
Ох уже это Copy-Paste программирование.
static int
test_tls_record_proto_all(const struct tls_record_test_flags *flags)
{
....
if (flags->zero_len &&
((flags->content_type == TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE) ||
(flags->content_type == TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE) ||
(flags->content_type == TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE))) {
if (ret == TEST_SUCCESS)
return TEST_FAILED;
goto skip_decrypt;
}
....
}Предупреждение PVS-Studio:
V501 There are identical sub-expressions '(flags->content_type == TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE)' to the left and to the right of the '||' operator. test_cryptodev.c 12173
Тут всё просто. Два раза сравниваем переменную с одной и той же константой. Юнит-тест не проверяет какую-то ситуацию. Явно одна из констант должна быть другой.
Вспоминается моя собственная заметка "Ноль, один, два, Фредди заберёт тебя". Только тут 6.
static int
test_missing_c_flag(void)
{
....
if (rte_lcore_is_enabled(0) && rte_lcore_is_enabled(1) &&
rte_lcore_is_enabled(2) && rte_lcore_is_enabled(3) &&
rte_lcore_is_enabled(3) && rte_lcore_is_enabled(5) &&
rte_lcore_is_enabled(4) && rte_lcore_is_enabled(7) &&
launch_proc(argv29) != 0) {
....
}Предупреждение PVS-Studio:
V501 There are identical sub-expressions 'rte_lcore_is_enabled(3)' to the left and to the right of the '&&' operator. test_eal_flags.c 678
В начале программист вызывал функцию rte_lcore_is_enabled, методично указывая константы: 0, 1, 2, 3.
А потом пошёл в разнос: 3, 5, 4, 7. То ли спешил, то ли его кто-то отвлёк.
На обзоре кода такую ошибку сложно заметить. Вроде, какие-то разные числа. Наверное, правильно. Вчитываться в такой код скучно. В результате юнит-тест не проверят вызов функции для значения 6.
Хорошо, что есть анализатор PVS-Studio, который не ленится внимательно присмотреться к каждой строчке и константе :) А для внимательного читателя у меня в середине статьи припасён промокод dpdk_6 на месячный триал. Удачной охоты за ошибками в коде.
int
port_meter_policy_add(portid_t port_id, uint32_t policy_id,
const struct rte_flow_action *actions)
{
....
for (i = 0; i < RTE_COLORS; i++) {
for (act_n = 0, start = act;
act->type != RTE_FLOW_ACTION_TYPE_END; act++)
act_n++;
if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
policy.actions[i] = start;
else
policy.actions[i] = NULL;
act++;
}
....
}Предупреждение PVS-Studio:
V560 A part of conditional expression is always true: act->type == RTE_FLOW_ACTION_TYPE_END. config.c 2249
Проверка act->type == RTE_FLOW_ACTION_TYPE_END не имеет смысла, так как расположенный выше цикл остановится только в случае, когда это условие выполнится.
Проверка лишняя, или проверяется не то условие.
static int
comp_names_to_index(struct context *ctx, const struct token *token,
unsigned int ent, char *buf, unsigned int size,
const char *const names[], size_t names_size)
{
RTE_SET_USED(ctx);
RTE_SET_USED(token);
if (!buf)
return names_size;
if (names[ent] && ent < names_size)
return rte_strscpy(buf, names[ent], size);
return -1;
}Предупреждение PVS-Studio:
V781 The value of the 'ent' index is checked after it was used. Perhaps there is a mistake in program logic. cmdline_flow.c 12870
В функцию передаётся массив имён, размер этого массива и индекс имени, с которыми будут работать. В функции есть проверка, что индекс не выходит за границу массива имён. Есть проверка, что имя не является пустым: указатель на имя ненулевой.
Но вот последовательность этих проверок подкачала:
if (names[ent] && ent < names_size)Указатель на имя извлекается из массива ДО проверки индекса. В результате, возможен выход за границу массива.
Для юнит-теста такая ошибка не является страшной. А вообще, это очень серьёзный ляп!
ГОСТ Р 71207–2024 классифицирует выход за границу массива как критические ошибки.
Приведу функцию целиком, чтобы было понятно, почему я называю код странным.
static int
parse_lcore_dma(struct test_configure *test_case, const char *value)
{
struct lcore_dma_map_t *lcore_dma_map;
char *input, *addrs;
char *ptrs[2];
char *start, *end, *substr;
uint16_t lcore_id;
int ret = 0;
if (test_case == NULL || value == NULL)
return -1;
input = strndup(value, strlen(value) + 1);
if (input == NULL)
return -1;
addrs = input;
while (*addrs == '\0')
addrs++;
if (*addrs == '\0') {
fprintf(stderr, "No input DMA addresses\n");
ret = -1;
goto out;
}
....
}Предупреждение PVS-Studio:
V547 Expression '* addrs == '\0'' is always false. main.c 234
Цикл заканчивается только тогда, когда будет найден ненулевой символ.
while (*addrs == '\0')
addrs++;
if (*addrs == '\0') {Поэтому последующая проверка не имеет смысла. Указатель всегда будет указывать не на терминальный ноль.
Однако это вообще безумный код. Указатель addrs указывает на копию строки value, полученную с помощью вызова функции strndup.
Какой смысл пытаться пропустить в цикле все нулевые символы?
while (*addrs == '\0')
addrs++;Если строка пустая (в начале сразу записан '\0'), то произойдёт выход за границу массива.
Если строка непустая, то цикл сразу остановится, не выполнив ни одной итерации.
Быть может хотели написать так?
while (*addrs != '\0')
addrs++;Нет, это тоже не имеет смысла.
У меня есть подозрение, что цикл здесь лишний, и должно быть написано так:
input = strndup(value, strlen(value) + 1);
if (input == NULL)
return -1;
addrs = input;
if (*addrs == '\0') {
fprintf(stderr, "No input DMA addresses\n");
ret = -1;
goto out;
}Я уже рассматривал этот красивый баг у себя в телеграмм канале "Бестиарий программирования". Приглашаю подписаться. Спасибо.
#define MAX_NUM 1 << 20
static int
test_align(void)
{
....
for (p = 1; p <= MAX_NUM / 2; p++) { // <=
for (i = 1; i <= MAX_NUM / 2; i++) { // <=
val = RTE_ALIGN_MUL_CEIL(i, p);
if (val % p != 0 || val < i)
FAIL_ALIGN("RTE_ALIGN_MUL_CEIL", i, p);
val = RTE_ALIGN_MUL_FLOOR(i, p);
if (val % p != 0 || val > i)
FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
val = RTE_ALIGN_MUL_NEAR(i, p);
if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
& (val != RTE_ALIGN_MUL_FLOOR(i, p))))
FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
}
}
....
}Предупреждения PVS-Studio:
Не люблю макросы по разным причинам. Одна из них, что они сильно разрывают восприятие программы. Перед вами как раз хороший пример: по отдельности код смотрится нормально, а вместе получается фигня.
Блок кода из юнит-тестов — вроде OK.
Макрос MAX_NUM тоже на первый взгляд нормальный:
#define MAX_NUM 1 << 20Однако при подстановке в условие циклов получается выражение:
1 << 20 / 2Приоритет операции деления выше, чем сдвига. В результате происходит сдвиг не на 20, а на 10 бит: 1 << 10.
Посчитаем, сколько вариантов перебирается в циклах из-за ошибки:
(1 << (20 / 2)) * (1 << (20 / 2)) = 1024 * 1024 = 1048576
Сколько вариантов должно перебираться по задумке программиста:
((1 << 20) / 2) * ((1 << 20) / 2) = 524288 * 524288 = 274877906944
Итого, юнит-тест выполняет в 274877906944/1048576 = 262144 раз меньше проверок.
При обзоре кода такую ошибку заметить можно, но сложно. Нужно явно посмотреть, как написан макрос, в голове сделать подстановки, вспомнить про приоритет операций. Статический анализатор — очень хороший помощник в поиске подобных ошибок.
Чтобы исправить код, нужно добавить скобки в макрос:
#define MAX_NUM (1 << 20)Если мы пишем на C++, то вообще лучше использовать constexpr. Заодно приглашаю почитать статью "Дизайн и эволюция constexpr в C++".
Кстати, если исправить баг, возможно, проявится другая проблема с этим юнит-тестом. Вернее, выяснится, что он работает слишком медленно, и его надо переделывать.
В следующей статье я рассмотрю оставшиеся баги, замеченные мной при проверке DPDK. А пока скачайте PVS-Studio и посмотрите, что интересного найдётся в юнит-тестах ваших проектов. Если найдётся что-то забавное — пишите комментарии.