Безопасность и DevOps

DevOps системным инженерам: инструменты и не только

• Какие подходы лежат в основе, как новые инструменты вписываются в существующие подходы или меняют их.
• Как новые инструменты позволяют делать что-то лучше, быстрее, адекватнее и дают больше возможностей подключить безопасность.
• Какие подходы приходят на смену старым и какие инструменты позволяют их лучше реализовать.

DevOps в продуктовой разработке

DevOps на Chief-уровне

Взаимопонимание во главе угла

Всё это онлайн

• Конкуренция за внимание, потому что в отличие от очной конференции, на которую приезжаешь в специальной место, на онлайн-конференции участники остаются в привычном окружении со всеми отвлекающими чатами и прочими соцсетями.
• Отсутствие живого общения со всеми его невербальными составляющими.

• Вот такой ширины с точки зрения форматов: будут доклады, мастер-классы, воркшопы, дискуссии, групповые и индивидуальные консультации и это еще не все карты раскрыты.
• Вот такой высоты с точки зрения аудитории: ждём на конференции инженеров разных IT-специальностей, безопасников, CТО, менеджеров продуктов и владельцев бизнеса.
• А еще и глубины, потому что хардкорные вещи тоже обязательно будут.

DevOps Live пройдет в два этапа 29-30 сентября и 6-7 октября. До 15 августа еще можно подать доклад или забронировать билет перед повышением цены.

Следите за обновлениями в телеграме или рассылке будем сообщать о принятых спикерах и открывать подробности форматов.

Всё, что осталось за кадром в этой беседе, можно будет уточнить 5 августа в 18:00 на онлайн-встрече программного комитета с докладчиками и сочувствующими. Подключайтесь и задавайте свои вопросы, уточняйте темы для заявок или давайте программному комитету задание закрыть свой запрос. Встреча открытая, но нужно зарегистрироваться, чтобы мы прислали ссылку.

Неправильный код, который работает

Размер выделяемой памяти

int main(int argc, char *argv[]){  ....  struct pool *pop = malloc(sizeof(pop));  ....}

struct pool *pop = malloc(sizeof(pool));

struct pool *pop = malloc(sizeof(*pop));

struct pool {  struct ctl *ctl;};

Длина строки

typedef void *(*pmem2_memcpy_fn)(void *pmemdest, const void *src, size_t len,    unsigned flags);static const char *initial_state = "No code.";static inttest_rwx_prot_map_priv_do_execute(const struct test_case *tc,  int argc, char *argv[]){  ....  char *addr_map = pmem2_map_get_address(map);  map->memcpy_fn(addr_map, initial_state, sizeof(initial_state), 0);  ....}

static const char initial_state [] = "No code.";

UT_ASSERTeq(memcmp(addr_map, initial_state, strlen(initial_state)), 0);

Побитовый сдвиг

static intclo_parse_single_uint(struct benchmark_clo *clo, const char *arg, void *ptr){  ....  uint64_t tmax = ~0 >> (64 - 8 * clo->type_uint.size);  ....}

Приоритет операций

#define BTT_CREATE_DEF_SIZE  (20 * 1UL << 20) /* 20 MB */

• Сдвинул 1 на 20 разрядов, чтобы получить значение 1048576, т.е. 1 MB.
• Умножил 1 MB на 20.

• (20 * 1UL << 20)
• (20 * (1UL << 20))
• ((20 * 1UL) << 20)

Другие ошибки

Не там поставленная скобка

#define STATUS_INFO_LENGTH_MISMATCH 0xc0000004static voidenum_handles(int op){  ....  NTSTATUS status;  while ((status = NtQuerySystemInformation(      SystemExtendedHandleInformation,      hndl_info, hi_size, &req_size)        == STATUS_INFO_LENGTH_MISMATCH)) {    hi_size = req_size + 4096;    hndl_info = (PSYSTEM_HANDLE_INFORMATION_EX)REALLOC(hndl_info,        hi_size);  }  UT_ASSERT(status >= 0);  ....}

while ((status = NtQuerySystemInformation(....) == STATUS_INFO_LENGTH_MISMATCH))

while ((status = NtQuerySystemInformation(....)) == STATUS_INFO_LENGTH_MISMATCH)

Потенциальная утечка памяти

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  if (!oidp)    return POCLI_ERR_PARS;  ....}

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  if (!oidp)    return POCLI_ERR_PARS;  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  ....}

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  if (!oidp)  {    free(input);    return POCLI_ERR_PARS;  }  ....}

Потенциальное переполнение

typedef long long os_off_t;voiddo_memcpy(...., int dest_off, ....., size_t mapped_len, .....){  ....  LSEEK(fd, (os_off_t)(dest_off + (int)(mapped_len / 2)), SEEK_SET);  ....}

LSEEK(fd, dest_off + (os_off_t)(mapped_len) / 2, SEEK_SET);

Неправильная защита от переполнения

static DWORDget_rel_wait(const struct timespec *abstime){  struct __timeb64 t;  _ftime64_s(&t);  time_t now_ms = t.time * 1000 + t.millitm;  time_t ms = (time_t)(abstime->tv_sec * 1000 +    abstime->tv_nsec / 1000000);  DWORD rel_wait = (DWORD)(ms - now_ms);  return rel_wait < 0 ? 0 : rel_wait;}

Отсутствие проверки, что память успешно выделена

static voidremove_extra_node(TOID(struct tree_map_node) *node){  ....  unsigned char *new_key = (unsigned char *)malloc(new_key_size);  assert(new_key != NULL);  memcpy(new_key, D_RO(tmp)->key, D_RO(tmp)->key_size);  ....}

static voidcalc_pi_mt(void){  ....  HANDLE *workers = (HANDLE *) malloc(sizeof(HANDLE) * pending);  for (i = 0; i < pending; ++i) {    workers[i] = CreateThread(NULL, 0, calc_pi,      &tasks[i], 0, NULL);    if (workers[i] == NULL)      break;  }  ....}

Код с запахом

Двойной вызов CloseHandle

static voidprepare_map(struct pmem2_map **map_ptr,  struct pmem2_config *cfg, struct pmem2_source *src){  ....  HANDLE mh = CreateFileMapping(....);  ....  UT_ASSERTne(CloseHandle(mh), 0);  ....}

#define UT_ASSERTne(lhs, rhs)\  do {\    /* See comment in UT_ASSERT. */\    if (__builtin_constant_p(lhs) && __builtin_constant_p(rhs))\      UT_ASSERT_COMPILE_ERROR_ON((lhs) != (rhs));\    UT_ASSERTne_rt(lhs, rhs);\  } while (0)

do {  if (0 && 0) (void)((CloseHandle(mh)) != (0));  ((void)(((CloseHandle(mh)) != (0)) ||    (ut_fatal(".....", 76, __FUNCTION__, "......: %s (0x%llx) != %s (0x%llx)",              "CloseHandle(mh)", (unsigned long long)(CloseHandle(mh)), "0",              (unsigned long long)(0)), 0))); } while (0);

((void)(((CloseHandle(mh)) != (0)) ||  (ut_fatal(...., "assertion failure: %s (0x%llx) != %s (0x%llx)",            ....., (unsigned long long)(CloseHandle(mh)), .... ), 0)));

Несоответствие интерфейса реализации (снятие константности)

static intstatus_push(PMEMpoolcheck *ppc, struct check_status *st, uint32_t question){  ....  } else {    status_msg_info_and_question(st->msg);            // <=    st->question = question;    ppc->result = CHECK_RESULT_ASK_QUESTIONS;    st->answer = PMEMPOOL_CHECK_ANSWER_EMPTY;    PMDK_TAILQ_INSERT_TAIL(&ppc->data->questions, st, next);  }  ....}

static inline intstatus_msg_info_and_question(const char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

char * strchr ( const char *, int );

static inline intstatus_msg_info_and_question(char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

Переусложненный код

static struct memory_blockheap_coalesce(struct palloc_heap *heap,  const struct memory_block *blocks[], int n){  struct memory_block ret = MEMORY_BLOCK_NONE;  const struct memory_block *b = NULL;  ret.size_idx = 0;  for (int i = 0; i < n; ++i) {    if (blocks[i] == NULL)      continue;    b = b ? b : blocks[i];    ret.size_idx += blocks[i] ? blocks[i]->size_idx : 0;  }  ....}

....for (int i = 0; i < n; ++i) {  if (blocks[i] == NULL)    continue;  b = b ? b : blocks[i];  ret.size_idx += blocks[i]->size_idx;}....

Подозрительное использование нулевого указателя

void win_mmap_fini(void){  ....  if (mt->BaseAddress != NULL)    UnmapViewOfFile(mt->BaseAddress);  size_t release_size =    (char *)mt->EndAddress - (char *)mt->BaseAddress;  void *release_addr = (char *)mt->BaseAddress + mt->FileLen;  mmap_unreserve(release_addr, release_size - mt->FileLen);  ....}

if (mt->BaseAddress != NULL)

size_t release_size =  (char *)mt->EndAddress - (char *)mt->BaseAddress;

Короткие имена глобальных переменных

static struct critnib *c;

• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'ri' variable. map.c 131
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'c' variable. obj_critnib_mt.c 56
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.h 68
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.c 34

Странное

voiddo_memmove(char *dst, char *src, const char *file_name,    size_t dest_off, size_t src_off, size_t bytes,    memmove_fn fn, unsigned flags, persist_fn persist){  ....  /* do the same using regular memmove and verify that buffers match */  memmove(dstshadow + dest_off, dstshadow + dest_off, bytes / 2);  verify_contents(file_name, 0, dstshadow, dst, bytes);  verify_contents(file_name, 1, srcshadow, src, bytes);  ....}

• Хотелось "потрогать" блок памяти. Но произойдёт ли это в реальности? Не удалит ли оптимизирующий компилятор код, который копирует блок памяти сам в себя?
• Это какой-то юнит-тест на работу функции memmove.
• Код содержит опечатку.

voiddo_memmove(char *dst, char *src, const char *file_name,    size_t dest_off, size_t src_off, size_t bytes,    memmove_fn fn, unsigned flags, persist_fn persist){  ....  /* do the same using regular memmove and verify that buffers match */  memmove(dstshadow + dest_off, srcshadow + src_off, 0);  verify_contents(file_name, 2, dstshadow, dst, bytes);  verify_contents(file_name, 3, srcshadow, src, bytes);  ....}

Зачем использовать анализаторы кода?

Принцип работы

cpe:2.3:part:vendor:product:version:update:edition:language:sw_edition:target_sw:target_hw:other

• Part: Указание о том, что компонент относится к приложению (a), операционной системе (o), железу (h) (Обязательный пункт)
• Vendor: Название производителя продукта (Обязательный пункт)
• Product: Название продукта (Обязательный пункт)
• Version: Версия компоненты (Устаревший пункт)
• Update: Обновление пакета
• Edition: Наследуемая версия (Устаревший пункт)
• Language: Язык, определяемый в RFC-5646
• SW Edition: Версия ПО
• Target SW: Программная среда, в которой работает продукт
• Target HW: Аппаратная среда, в которой работает продукт
• Other: Информация о поставщике или продукте

cpe:2.3:a:pivotal_software:spring_framework:3.0.0:*:*:*:*:*:*:*

scheme:type/namespace/name@version?qualifiers#subpath

• Sсheme: Всегда будет 'pkg', указывающий, что это URL-адрес пакета (Обязательный пункт)
• Type: Тип пакета или протокол пакета, например maven, npm, nuget, gem, pypi и т.д. (Обязательный пункт)
• Namespace: Некоторый префикс имени, такой как идентификатор группы Maven, владелец образа Docker, пользователь или организация GitHub.Необязательный и зависит от типа.
• Name: Имя пакета (Обязательный пункт)
• Version: Версия пакета
• Qualifiers: Дополнительные квалификационные данные для пакета, такие как ОС, архитектура, дистрибутив и т. Д. Необязательный и зависящий от типа пункт.
• Subpath: Дополнительный путь в пакете относительно корня пакета

pkg:golang/google.golang.org/genproto#googleapis/api/annotationspkg:maven/org.apache.commons/io@1.3.4pkg:pypi/django-package@1.11.1.dev1

<?xml version="1.0" encoding="UTF-8"?><bom xmlns="http://personeltest.ru/away/cyclonedx.org/schema/bom/1.2" serialNumber="urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79" version="1">  <components>    <component type="library">      <publisher>Apache</publisher>      <group>org.apache.tomcat</group>      <name>tomcat-catalina</name>      <version>9.0.14</version>      <hashes>        <hash alg="MD5">3942447fac867ae5cdb3229b658f4d48</hash>        <hash alg="SHA-1">e6b1000b94e835ffd37f4c6dcbdad43f4b48a02a</hash>        <hash alg="SHA-256">f498a8ff2dd007e29c2074f5e4b01a9a01775c3ff3aeaf6906ea503bc5791b7b</hash>        <hash alg="SHA-512">e8f33e424f3f4ed6db76a482fde1a5298970e442c531729119e37991884bdffab4f9426b7ee11fccd074eeda0634d71697d6f88a460dce0ac8d627a29f7d1282</hash>      </hashes>      <licenses>        <license>          <id>Apache-2.0</id>        </license>      </licenses>      <purl>pkg:maven/org.apache.tomcat/tomcat-catalina@9.0.14</purl>    </component>      <!-- More components here -->  </components></bom>

Dependency Check

Первый запуск

mvn org.owasp:dependency-check-maven:check

Dependency Track

Установка

Первый запуск

mvn org.cyclonedx:cyclonedx-maven-plugin:makeAggregateBom

Nexus IQ

Первый запуск

mvn com.sonatype.clm:clm-maven-plugin:evaluate -Dclm.applicationId=dvja-test-and-compare -Dclm.serverUrl=<NEXUSIQIP> -Dclm.username=<USERNAME> -Dclm.password=<PASSWORD>

• Dependencies Scanned:62
• Vulnerable Dependencies:16
• Vulnerabilities Found:42 (8 sonatype db)

• Dependencies Scanned:47
• Vulnerable Dependencies:13
• Vulnerabilities Found:91 (14 sonatype oss)

• Dependencies Scanned: 59
• Vulnerable Dependencies:10
• Vulnerabilities Found:51 (1 sonatype oss)

Дисклеймер

Сравнение результатов

• Несоответствие CVE к выявленной компоненте
• Например, если уязвимость выявлена во фреймворке struts2, а инструмент указывает на компоненту фреймворка struts-tiles, к которой эта уязвимость не относится, то это false positive
• Несоответствие CVE к выявленной версии компоненты
• Например, уязвимость привязана к версии python > 3.5 и инструмент отмечает уязвимой версию 2.7 это false positive, так как на самом деле уязвимость относится только к ветке продукта 3.x
• Дублирование CVE
• Например, если SCA указал на CVE, позволяющую реализовать RCE, после чего SCA указывает для этой же компоненты CVE, применимую к продуктам Cisco, подверженным этой RCE. В таком случае будет false positive.
• Например, CVE была найдена в компоненте spring-web, после чего SCA указывает на эту же CVE в других компонентах фреймворка Spring Framework, в то время как CVE к другим компонентам отношения не имеет. В таком случае будет false positive.

Сводные результаты

Подробнее

1

2

3

4

5

6

Выводы

Почему я выбрал open source инструмент для статического анализа безопасности кода

Использование Graudit для анализа безопасности кода

$ git clone https://github.com/wireghoul/graudit

$ cd ~/bin && mkdir graudit$ ln --symbolic ~/graudit/graudit ~/bin/graudit

#------ .bashrc ------alias graudit="~/bin/graudit"

$ source ~/.bashrc # OR$ exex $SHELL

$ graudit -h

$ graudit -d ~/gradit/signatures/js.db

Преимущества и недостатки Graudit

Начало...

На правах рекламы

Узнав результаты голосования, проведённого в одной из наших прошлых статей, мы решили более подробно обсудить вопрос организации фаззинга. Кроме того, в рамках онлайн-встречи по информационной безопасности "Digital Security ON AIR" мы представили доклад, основанный на нашем опыте в DevSecOps, где также рассказали об этой интересной теме.

Записи всех докладов можно посмотреть на нашем Youtube-канале. Если же вы предпочитаете текстовый формат, добро пожаловать под кат!

1. Фаззинг
   
   • Введение
   • Эффективно ли тестирование?
   • Что такое фаззинг?
   • Фаззинг всё ещё популярен?
   • Статический анализ
   • Непрерывный фаззинг в непрерывной разработке
2. Этапы интеграции фаззинга в проект
   
   • Выбор инструментов для организации фаззинга
   • Определение Attack surface
   • Анализ целей для фаззинга
   • Подбор входных данных
   • Написание фаззера
   • Сборка и её особенности на разных платформах
3. Фаззинг в облаке
   
   • Фаззинг-ферма от Google
   • Почему решение от Google подходит не всем?
   • Что получаем в итоге?
4. Заключение

Фаззинг

Введение

Относительно недавно вышла замечательная книга Building Secure and Reliable Systems от инженеров компании Google. Её главным лейтмотивом является мысль о том, что безопасность и надёжность систем и ПО тесно взаимосвязаны. Публикация подобного материала одним из крупнейших лидеров IT-рынка отлично демонстрирует, что безопасность становится неотъемлемой частью как процесса разработки, так и всего жизненного цикла системы или ПО.

Эффективно ли тестирование?

Чтобы сделать безопасную и стабильную систему, её нужно тестировать. К сожалению, писать идеальный код, не требующий доработки, не может никто. Поэтому в написанном коде априори есть ошибки и уязвимости. Как и в других сферах жизни, понимание этого приходит лишь с опытом. Аналогично в IT в целом и в разработке в частности: каждая компания или команда может находиться на своем уровне профессиональной зрелости. Какие-то подходы на определённом этапе будут просто недоступны для них. Так, если не внедрена непрерывная интеграция, то и о нормальном тестировании речи быть не может, не говоря уже о тестировании безопасности во время разработки. Так что, если вы не внедрили непрерывную интеграцию (CI), мы надеемся, что после прочтения нашей статьи вы начнёте делать шаги в нужном направлении. Далее мы предполагаем, что у вас есть настроенный и функционирующий CI, на базе которого и будут предлагаться улучшения.

CI/CD нужны не только для автоматизированных тестов, но и в глобальном плане для того, чтобы соответствовать требованиям времени. Каскадная модель разработки, согласно которой этапы проектирования, разработки и тестирования происходят строго последовательно, постепенно уходит в прошлое или остается лишь в определенных нишах. Сейчас очень важна скорость разработки. Важно, чтобы каждая часть ПО могла развиваться независимо от других частей. Таким требованиям соответствует метод непрерывной разработки. А непрерывной разработке нужно непрерывное тестирование.

Но перед тем, как продолжить, нам хотелось бы напомнить, что чем раньше найдена проблема, тем проще и менее затратно её исправить как во временном, так и в денежном эквиваленте. С точки же зрения обнаружения уязвимостей, это ещё и временные затраты на доставку патчей до ваших клиентов, а также их своевременная установка. Ведь закрытие неизвестной уязвимости нельзя заранее запланировать. А отвлекать разработчиков по малейшему поводу от их основной деятельности разработки продукта это тоже терять время и деньги. И ваши клиенты не будут рады часто прилетающим экстренным патчам. Равно как и разработчики :)

https://resources.securitycompass.com/blog/how-to-sell-training-costs-internally-2

Традиционная практика тестирования предполагает, что на вход программы подаются самые разные данные: корректные, почти корректные и граничные случаи. Это позволяет убедиться, что код работает в целом правильно и серьёзных ошибок в логике работы нет. Однако при этом качество проверки зависит от качества тестов и их количества. Фиксированное количество тестов не может покрыть все возможные варианты входных данных. Поэтому всегда есть вероятность, что какой-то важный случай был упущен, не вошёл в фиксированный набор тестов, а код, следовательно, всё-таки содержит ошибку, которую достаточно талантливый хакер сможет проэксплуатировать. Так что отсутствие падений или нестандартного поведения ПО в результате тестирования не следует рассматривать как повод его прекратить. Впрочем, и слепо перебирать все возможные значения это тоже не вариант. На это просто не хватит никаких ресурсов.

Именно здесь в игру вступает фаззинг. Он берет лучшее от двух существующих подходов. Мы не ограничиваем себя конечным количеством тестовых случаев и постоянно генерируем случайные входные данные для нашего кода, но при этом каждая новая последовательность использует информацию из предыдущих попыток для максимизации результата.

Что такое фаззинг?

Для начала небольшой исторический экскурс. Сам термин "фаззинг" ("fuzzing") появился впервые около 30 лет назад в работе An Empirical Study of the Reliability of UNIX Utilities. С его появлением связана одна интересная легенда. В 1988 удар молнии исказил передаваемые по линии данные, что привело к падению утилиты автора исследования. После этого уже профессор Брет Миллер вместе со своими студентами сделал полноценное исследование в данном направлении и на одном из семинаров представил программу fuzzer. Данная программа предназначалась как раз для тестирования ПО. Сам Брет Миллер так описывал свой подход: Наш подход не является заменой формальной верификации или тестирования. Скорее, это легковесный механизм для поиска ошибок в системе и повышения её надёжности. Официальное же определение фаззинга звучит так:

Фаззинг это техника автоматизированного тестирования, при которой на вход программе подаются специально подготовленные данные, которые могут привести её к аварийному состоянию или неопределённому поведению.

Сегодня мы сконцетрируемся на использовании фаззинга для кода на таких языках программирования, как С и С++, потому что:

• в ПО на таких ЯП чаще находят бинарные уязвимости;
• эффективные средства поиска уязвимостей, такие как libFuzzer и AFL, ориентированы, в первую очередь, на языки C и C++;
• огромное количество программ и библиотек написано на C и C++.

Тем не менее все указанные наработки как минимум можно использовать без особых проблем для ПО на Rust или Go.

К сожалению, до сих пор в индустрии нет чётких определений и классификаций фаззеров. На рисунке ниже вы можете видеть попытку описать существующие вариации популярных фаззеров.

The Art, Science, and Engineering of Fuzzing:A Survey

Поэтому от работы к работе вы будете встречать одни и те же термины в разных контекстах. В рамках наших исследований безопасности мы классифицируем фаззеры следующим образом:

• по знанию о входных данных:
  
  • Black-box мы ничего не знаем про формат данных. Нужно реверсить
  • Gray-box что-то о формате мы выяснили, но полная информация о нём нам всё ещё неизвестна
  • White-box у нас есть спецификация. Мы располагаем всеми сведениями о формате данных
• по цели, которая будет подвергнута фаззингу:
  
  • source-based у нас есть исходники проекта
  • binary-based у нас нет исходников проекта
• по наличию обратной реакции от тестируемого приложения:
  
  • feedback driven есть обратная реакция
  • not feedback driven нет обратной реакции
• по операциям, которые будут совершаться над входными данными:
  
  • генерационные
  • мутационные
  • комбинированные

Стоит отметить, что часто binary-based фаззеры называют black-box, а source-based gray- или white-box. В данной статье нас в первую очередь интересует source-based фаззинг с обратной связью, поскольку мы строим DevSecOps для разработки собственного продукта или анализа opensource-проектов, а это значит, что исходный код для него у нас есть.

Фаззинг всё ещё популярен?

Кто-то может задаться вопросом: "Если фаззингу уже более 30 лет (согласно некоторым источникам и с учетом прообраза этого метода тестирования, и все 70), то почему только в последнее время он стал так популярен?".

В начале тысячелетия Microsoft в рамках описания создания Secure Development Lifecycle
выпустили несколько статей, посвящённых тематике фаззинга, но тогда речь в основном шла о black-box-фаззинге. Однако в 2013 году фаззинг получил вторую жизнь, благодаря созданию feedback-driven fuzzing, который позволил достичь новых высот в этой сфере. Важную роль также сыграло появление простых и понятных инструментов, о которых мы расскажем далее.

Feedback-driven fuzzing это вид фаззинга, при котором фаззер изменяет входные данные так, чтобы их обработка затрагивала как можно больше участков кода программы. Работа таких фаззеров возможна, благодаря их способности реагировать на отклик (feedback) программы. Обычно таким откликом является покрытие кода. Метрики покрытия кода отслеживают суммарное количество выполненных строк кода, базовых блоков, количество сделанных условных переходов. Задача фаззера генерировать данные, которые приводят к увеличению покрытия кода.

Одним из наиболее популярных feedback-driven фаззеров является American Fuzzy Lop от Michael Zalewski. AFL был создан в 2013 году и стал главным двигателем прогресса в современном фаззинге.

Про AFL было уже несколько статей на Хабре, но повторимся, как он работает:

• в исполняемый файл добавляется инструментация для сбора информации о покрытии кода. Информация будет сохраняться в shared bitmap;
• AFL запускает программу и доходит до функции, которую нужно профаззить. Здесь он сохраняет своё состояние через системный вызов fork;
• далее происходит цикл "мутация данных запуск откат". При этом данные мутируются с учётом покрытия кода, получаемого от программы.

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

AFL оказался чрезвычайно эффективным при фаззинге различных программ. Это и сделало его знаменитым. Он был добавлен в Google Summer of Code (GSoC), а вокруг него сформировалось большое сообщество. Многочисленные энтузиасты предлагают собственные модификации AFL, которые по разным причинам не были приняты в основную ветку (например, существуют вариации для других ЯП). Об этом можно узнать подробнее в ещё одной нашей статье, посвященной различным вариациям AFL фаззера. Ещё хотелось бы заметить, что создатель AFL отошёл от дел и до недавнего времени проект почти не обновлялся, из-за чего многие стали использовать его активно поддерживаемый форк AFL++.

Хотя AFL и произвёл революцию в фаззинге, для разработчиков он всё равно долгое время оставался больше хакерской тулзой, непригодной для использования в процессе разработки. Но трудно не заметить эффективность такого подхода, поэтому можно было не сомневаться, что его адаптируют и для нужд разработчиков.

В итоге так и случилось. В 2015 году появился libFuzzer один из подпроектов LLVM. LibFuzzer позволяет реализовать feedback-driven in-memory фаззинг и имеет все преимущества, характерные для средств фаззинга исходного кода:

• позволяет создавать отдельные фаззеры для каждой функции из рабочего проекта;
• использует санитайзеры;
• может быть интегрирован в проект через toolchain.

Статический анализ

Прежде чем обратиться непосредственно к Continuous Fuzzing, скажем пару слов об анализаторах кода и фаззерах. Для тех, кто думает, нужно ли это всё, если те или иные анализаторы кода уже используются в процессе разработки.

Анализаторы кода относятся к статическому тестированию. Они обладают почти стопроцентным покрытием кода при анализе, но при этом и большим количеством ложно-позитивных срабатываний в купе с небольшим набором детектов, который ограничивается, по сути дела, лишь теми, что занесены в базу знаний.

Фаззинг относится к динамическому тестированию. Он обеспечивает покрытие кода в зависимости от качества предоставленных тестовых данных и собственных алгоритмов мутации тестов. Все его находки это точно проблемы, которые можно сразу воспроизвести.

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

На эту тему есть отличная статья How to Prevent the next Heartbleed, в которой автор сравнил эти подходы для улучшения качества openssl на примере нашумевшей уязвимости heartbleed. И именно подход, объединяющий анализ кода и фаззинг, помог выявить проблему с heartbleed. А обычное покрытие кода тестами, пусть и 100% по всем веткам разработки, оказалось не столь эффективным.

Непрерывный фаззинг в непрерывной разработке

Что же такое Continuous Fuzzing? Continuous fuzzing это подход к организации процесса. Иными словами, его эффективность будет зависеть от правильности организации процесса разработки в вашей компании. Все инструменты для организации такого процесса уже давно созданы осталось лишь правильно встроить их в свою систему.

К сожалению, многие разработчики и QA-тестировщики не так пристально следят за тенденциями в мире информационной безопасности. Поэтому для них лучше всего сделать так, чтобы процесс тестирования безопасности был максимально простым и прозрачным. Он не должен требовать от них особых знаний в этой сфере и сильно влиять на их трудовые будни. Правильно встроенный в процесс разработки continuous fuzzing не отвлекает и работает достаточно просто: написал код, сделал тест, подготовил тестовые данные и всё. Остальное сделает система. Настроенная среда разработки и отлаженные процессы будут служить вам годами, а разработчики приходить и уходить. Таким образом, лучше всё автоматизировать и не зависеть от конкретных людей.

Крупные компании уже не первый год настраивают у себя процесс фаззинга и развивают используемые инструменты. Этот подход уже множество раз доказал свою эффективность. Одна только статистика от Google ошеломляет числом найденных за время ее работы уязвимостей. По данным на 2019-й год, за все время существования ClusterFuzz было обнаружено около 16000 ошибок в Chrome и 11000 ошибок в более чем 160 популярных проектах с открытым исходным кодом.

Для организации фаззинга большие компании используют разный инструментарий. Выбор инструментов зависит от того, какие цели преследуются фаззингом, а также насколько безболезненно возможно внедрить этот инструментарий в уже существующий процесс разработки. В основе ClusterFuzz от Google лежат LibFuzzer, AFL и HonggFuzz (создан на базе LibFuzzera). Компания Microsoft недавно сделала публичным свой проект Springfield, превратив его в полноценный SaaS. Mozilla, помимо уже упомянутых фаззеров, разработала свой фреймворк для быстрого построения фаззеров (Grizzly) и дашборд для агрегации результатов фаззинга. Github не отстает. Так, он интегрировал ossfuzz в свою систему.

Вы, конечно, можете сказать, что это всё довольно большие компании, которые могут выделить значительные "лишние" мощности на "генерацию случайных данных". Но что мы видим? Несмотря на работу ClusterFuzza, та же компания Google активно поддерживает сторонних разработчиков и исследователей безопасности, позволяя запускать написанные ими фаззеры на своих мощностях. А если в процессе были обнаружены баги, за каждый из них выплачивается денежное вознаграждение. Всё-таки предварительный анализ и творческий подход исследователя безопасности позволяют найти новые векторы и уязвимости там, где "бездушная машина" заходит в тупик.

Поэтому, чтобы начать строить процесс Continuous Fuzzing у себя, необязательно быть крупной компанией, как тот же Google. Для этого не нужно крупных кластеров или мейнфреймов стартовые требования для внедрения Continuous Fuzzing минимальны. Речь о них пойдёт ниже, так как мы в своих проектах чаще используем значительно меньшие мощности и при этом успешно находим уязвимости.

Этапы интеграции фаззинга в проект

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

Выбор инструментов для организации фаззинга

Начнём со списка инструментов, которые необходимы для организации фаззинга, и разделим их условно на 3 большие категории:

• Составление поверхности атаки (Attack Surface)
  
  • ПО, в котором удобнее проводить аудит кода (Пример: Sourcetrail)
  • ПО для построения MindMap-схем (Пример: Xmind)
• Фаззинг
  
  • AFL (форк AFL++)
  • LibFuzzer (HonggFuzz)
  • KLEE*
• Санитайзеры (Sanitizers)

Для составления поверхности атаки необходимо ПО для аудита кода. Например, это может быть sourcetrail, understand, или подойдёт какая-нибудь популярная IDE. Лучше, чтобы ПО для инспекции кода имело возможность автоматического "рисования" блок-схем. Если у разработчиков уже есть готовые блок-схемы для программ из их проекта, то это значительно облегчит процесс. Если же нет, то искренне соболезнуем. Далее из составленной блок-схемы необходимо извлечь те куски, где тем или иным способом могут попасть на вход данные из недоверенных источников. Так и определяется поверхность атаки. На практике весьма удобно оформлять поверхность атаки в виде майндмап. Поверхность атаки служит не только для составления метрик "безопасности" (code coverage), а также, можно сказать, выступает чеклистом для написания фаззеров.

Об AFL и libFuzzer мы уже рассказали ранее, а практическое применение рассмотрим дальше, поэтому не будем на них останавливаться подробно. Можно считать их стандартом индустрии на данный момент. KLEE не совсем фаззер, хотя в некотором роде его можно использовать и так. Для описания процесса работы KLEE потребуется отдельная статья (на Хабре можно найти несколько, но KLEE за это время сильно обновился). Если же кто-то хочет углубиться в тему SMT, то в одной из прошлых статей мы опубликовали список материалов, который может помочь в этом. Если вкратце, то KLEE представляет собой символьную виртуальную машину, где параллельно выполняются символьные процессы и где каждый процесс представляет собой один из путей в исследуемой программе. Для каждого уникального пройденного пути KLEE сохраняет набор входных данных, необходимых для прохождения по этому пути. Это помогает улучшить эффективность фаззинга с помощью увеличения покрытия, а следовательно он прекрасно работает в паре с самими фаззерами.

Следующий тип инструментов санитайзеры. Ведь мало просто сгенерировать фаззером некорректные входные данные, которые обрушат программу. Нужно эти ошибки ещё и устранить. Санитайзеры позволяют поймать неуловимые ошибки, например, связанные с утечками памяти, которые бывает очень проблематично найти. Помимо этого, они позволяют собрать максимум информации о падении программы. По сути дела, это инструменты для динамического тестирования и их можно использовать раздельно, но именно их совместное использование с фаззерами значительно повышает эффективность тестирования. Так что в некотором смысле фаззеры и санитайзеры созданы друг для друга.

Сегодня работу с санитайзерами поддерживают компиляторы GCC и Clang. Настоятельно рекомендуем обратить на них внимание и попробовать, если еще не пробовали.

Мы не зря начали наш список инструментов с тех, что помогают составить Attack Surface. В презентации для FuzzCon, посвящённой 25-летию фаззинга, была представлена следующая схема:

Из неё хорошо видно, что это необходимый предварительный этап для организации качественного фаззинга. Далее идут этапы определения входных данных, непосредственно самого фаззинга и, конечно же, разбора результатов. Давайте рассмотрим каждый из них отдельно.

Определение Attack Surface

Что же стоит за словосочетанием "attack surface" ("поверхность атаки")? Возможно, вы уже знаете официальное значение, так как оно так прочно обосновалось в глоссарии, что даже удостоилось отдельной страницы в словаре хакерских терминов журнала Wired. Однако повторимся: если говорить по простому, это "возможность получения некорректных данных из недоверенных источников".

Помимо фаззинга, представление о поверхности атаки пригодится и просто для повышения уровня безопасности, поскольку оно позволяет полностью пересмотреть ПО и определить наличие лишних элементов. Самое частое и до сих пор встречающееся это обнаружение лишних или устаревших функций, к примеру, отладочных интерфейсов/API и т.п.

Точки входа поверхности атаки можно разделить на следующие категории. Основные это две (можно заметить, что они очень схожи с ранее упомянутыми типами фаззеров):

• файлы различные парсеры (XML, JSON), мультимедиакодеки (аудио, видео, графика);
• сеть сетевые протоколы (HTTP, SMTP), криптография (SSL/TLS), браузеры (Firefox, Chrome) и архиваторы файлов (ZIP, TAR).

Опциональными категориями в некоторых случаях можно выделить ядро или код общего назначения, например gui.

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

В зависимости от структуры проекта количество нод может быть даже меньше, но в данном случае мы имеем следующее разделение:

• точки входа
• модули
• классы, в некоторых случаях модуль это и есть класс
• непосредственно сами файлы с функциями, которые и будут фаззиться.

Иногда фаззер будет взаимодействовать сразу с несколькими функциями. Например, входные данные поступают только после распаковки файла, поэтому верным решением будет задействовать весь модуль.

Анализ целей для фаззинга

Цель для фаззинга (fuzz target) это функция, которая принимает на вход данные и обрабатывает их с использованием тестируемого API. Иными словами, это то, что нам необходимо фаззить.

Данный этап заключается в тщательном анализе каждой цели для фаззинга из attack surface. Вот что необходимо узнать:

1. Аргументы функции, через которые передаются данные для обработки. Нужен сам буфер для данных и его длина, если её возможно определить.
2. Тип передаваемых данных. Например, документ html, картинка png, zip-архив. От этого зависит то, как будут генерироваться и мутироваться поступающие на вход данные.
3. Список ресурсов (память, объекты, глобальные переменные), которые должны быть инициализированы перед вызовом целевой функции.
4. Если мы фаззим внутренние функции компонентов, а не API, то понадобится составить список ограничений, которые накладываются на данные кодом, выполненным ранее. Бывает так, что проверка данных происходит в несколько этапов это нам тоже следует учитывать.

Анализ opensource-проекта с использованием sourcetrail

Этот этап является самым кропотливым, ведь целей для фаззинга может быть очень много: сотни или даже тысячи! Как раз тут у нас появился термин "реверс исходников", поскольку времени и сил на анализ может быть потрачено примерно столько же, сколько уходит на реверс приличного бинарного файла.

Подбор входных данных

Перед тем, как начать фаззить, необходимо выбрать набор входных данных, который послужит отправной точкой для фаззера сиды (seeds). По своей сути сиды это папка с файлами, содержимое которых должно быть валидным, с точки зрения целевой программы или функции. Сиды будут подвергаться многочисленным мутациям в процессе фаззинга и приводить к росту покрытия кода.

Для каждой из функций, которые мы будем фаззить, должны быть свои сиды. Частенько мы заимствуем их из тестов проекта. Но если сэмплов из тестов не хватает, то всегда можно найти что-нибудь в интернете.

При создании набора seeds нужно учитывать, что:

• Его элементы должны влиять на покрытие кода программы. Чем выше покрытие, тем меньше неисследованных мест остаётся в программе.
• Размер его элементов не должен быть большим, иначе это повлияет на скорость фаззинга. Ведь чем больше длина входных данных, тем дольше функция будет их обрабатывать и меньше запусков программы сможет сделать фаззер за единицу времени.
• Кейсы в сидах должны быть функционально различными. Сильно похожие данные будут тормозить процесс фаззинга, поскольку фаззер часто будет попадать в места, которые уже были им исследованы. Перед запуском фаззера лучше провести минимизацию данных и убрать всё лишнее.

В процессе фаззинга сиды трансформируются в корпус. Корпус (corpus) это набор тест-кейсов, которые привели к росту покрытия кода во время фаззинга целевой программы или функции. Иначе говоря, это те самые интересные инпуты, которые потенциально могут привести к крешу программы. В корпусе сначала содержатся сиды, потом их мутации, затем мутации мутаций и т.д. Здесь мы видим, что фаззинг (feedback-driven) это циклический процесс, где на каждой новой итерации у нас всё больше шансов сгенерировать набор входных данных, который позволит найти уязвимости в программе.

Сиды очень сильно влияют на эффективность фаззинга. Этап подбора входных данных также нельзя игнорировать. Однако современные фаззеры порой бывают настолько хороши, что могут буквально из ничего сгенерировать хороший корпус. Пример такого корпуса, сгенерированного фаззером AFL, показан на рисунке выше. Более подробно этот процесс автор AFL Michal Zalewski описал в своём блоге. Советуем почитать.

Написание фаззеров

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

Допустим, у нас есть функция, которую надо фаззить. Для этого мы скачиваем набор библиотек llvm, среди которых есть компилятор clang и санитайзеры. Далее пишем код для фаззера:

#include "MyAPI.h"extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {  MyAPI_ProcessInput(Data, Size);  return 0;}

Данный код скомпилируется в отдельный исполняемый файл, где в цикле на вход функции MyAPI_ProcessInput будут подаваться случайные данные Data размером Size. Если фаззер обнаружит ошибку, то на консоль будет выведено сообщение о ней. Настанет время устранить ошибку, а после запустить фаззинг дальше.

<!--INFO: Seed: 2240819152INFO: Loaded 1 modules   (6 inline 8-bit counters): 6 [0x565e90, 0x565e96), INFO: Loaded 1 PC tables (6 PCs): 6 [0x541908,0x541968), INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytesINFO: A corpus is not provided, starting from an empty corpus#2  INITED cov: 3 ft: 4 corp: 1/1b lim: 4 exec/s: 0 rss: 35Mb===================================================================29562==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff2fba1ba0 at pc 0x0000004dda61 bp 0x7fff2fba1b30 sp 0x7fff2fba12d0WRITE of size 65 at 0x7fff2fba1ba0 thread T0    #0 0x4dda60 in strcpy (/home/user/Documents/tmp/main+0x4dda60)    #1 0x52540f in MyAPI_ProcessInput(char const*) (/home/user/Documents/tmp/main+0x52540f)    #2 0x52561e in LLVMFuzzerTestOneInput (/home/user/Documents/tmp/main+0x52561e)    #3 0x42fe0a in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/user/Documents/tmp/main+0x42fe0a)    #4 0x42f3a5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/home/user/Documents/tmp/main+0x42f3a5)    #5 0x4310ee in fuzzer::Fuzzer::MutateAndTestOne() (/home/user/Documents/tmp/main+0x4310ee)    #6 0x431dc5 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) (/home/user/Documents/tmp/main+0x431dc5)    #7 0x427df0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/user/Documents/tmp/main+0x427df0)    #8 0x44b402 in main (/home/user/Documents/tmp/main+0x44b402)    #9 0x7f7ee294409a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)    #10 0x421909 in _start (/home/user/Documents/tmp/main+0x421909)Address 0x7fff2fba1ba0 is located in stack of thread T0 at offset 96 in frame    #0 0x5252ef in MyAPI_ProcessInput(char const*) (/home/user/Documents/tmp/main+0x5252ef)  This frame has 1 object(s):    [32, 96) 'buf' <== Memory access at offset 96 overflows this variableHINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork      (longjmp and C++ exceptions *are* supported)SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/user/Documents/tmp/main+0x4dda60) in strcpyShadow bytes around the buggy address:  0x100065f6c320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  0x100065f6c330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  0x100065f6c340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  0x100065f6c350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  0x100065f6c360: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 00=>0x100065f6c370: 00 00 00 00[f3]f3 f3 f3 00 00 00 00 00 00 00 00  0x100065f6c380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  0x100065f6c390: f1 f1 f1 f1 00 00 00 00 f2 f2 f2 f2 f8 f3 f3 f3  0x100065f6c3a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  0x100065f6c3b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  0x100065f6c3c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00Shadow byte legend (one shadow byte represents 8 application bytes):  Addressable:           00  Partially addressable: 01 02 03 04 05 06 07   Heap left redzone:       fa  Freed heap region:       fd  Stack left redzone:      f1  Stack mid redzone:       f2  Stack right redzone:     f3  Stack after return:      f5  Stack use after scope:   f8  Global redzone:          f9  Global init order:       f6  Poisoned by user:        f7  Container overflow:      fc  Array cookie:            ac  Intra object redzone:    bb  ASan internal:           fe  Left alloca redzone:     ca  Right alloca redzone:    cb  Shadow gap:              cc==29562==ABORTINGMS: 1 InsertRepeatedBytes-; base unit: adc83b19e793491b1c6ea0fd8b46cd9f32e592fc0xa,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,\x0a\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xffartifact_prefix='./'; Test unit written to ./crash-76387a0aaeb6a1d2b6b6f095ab49c927c00243e5-->

Сборка и её особенности на разных платформах

Сделать сборку продукта вместе с фаззерами может оказаться более масштабной задачей, чем кажется на первый взгляд. Есть три основных момента, на которые надо обратить внимание: платформа, компилятор, система сборки.

Платформа

Вопрос о выборе платформы для фаззинга упирается в работоспособность инструментов на каждой из них. На данный момент whitebox-тестирование намного лучше показывает себя на unix-системах.

Однако, если продукт работает лишь на ОС Windows, не стоит отчаиваться. Во-первых, инструментарий для фаззинга активно развивается. Во-вторых, код крупных проектов, как правило, не является платформозависимым. Можно собрать необходимую часть проекта на unixсистеме и начать его фаззить. Ошибки никуда не денутся.

Компилятор

Думая о том, каким компилятором собирать фаззеры, мы всегда вспоминаем, что libfuzzer это подпроект LLVM, а в AFL предусмотрен llvm_mode, про который в его readme написано:

Настоящая инструментация на этапе компиляции, а не просто грубые изменения на уровне ассемблера.

Возможность добавления инструментации во время компиляции и поддержка LLVM самыми популярными фаззерами делает выбор компилятора очевидным для сборки лучше всего использовать clang. Конечно, нам попадались проекты, которые не были ориентированы на сборку этим компилятором, но нас это никогда не останавливало.

Система сборки

Касательно системы сборки, отметим, что это, скорее, вопрос вкуса и, возможно, привычки использования того или иного инструмента. Конкретно наш опыт показывает, что использование cmake позволяет в разы ускорить внедрение фаззинга в проект:

# Step 1add_custom_target(${FUZZ_TARGET_NAME})# Step 2function(add_fuzzer name path)    add_executable(${name} ${SRC} ${path})    target_compile_options(...)    set_target_properties(...)    add_dependencies(${FUZZ_TARGET_NAME} ${name})# Step 3set(fuzzers my_ideal_fuzzer1 my_ideal_fuzzer2 ...)foreach(fuzzer ${fuzzers})    add_fuzzer(${fuzzer} ${FUZZERS_DIR})endforeach()

Кратко интеграцию фаззеров в сборку проекта на cmake можно описать в трех шагах на примере нашего недавнего проекта:

• Создаём цель ${FUZZ_TARGET_NAME}, к которой в зависимости будем добавлять фаззеры.
• Пишем простую функцию, которая принимает на вход название фаззера и путь к его исходному коду. Реализация этой функции сводится к нескольким вызовам других cmake-функций. Сначала необходимо указать список исходников для сборки компонента который мы будем фаззить. Он хранится в переменной ${SRC}. Потом идут два вызова для установки опций компилятора и компоновщика. Последним вызовом добавляем новый исполняемый файл в качестве зависимости к цели, созданной ранее.
• Остаётся дописать простой цикл который поочередно добавит все фаззеры к созданной ранее цели.

Другой пример система сборки autotools. Она является достаточно старой и не такой удобной, как cmake. Однако это не помешает внедрению фаззинга в проект. Например, здесь показан пример интеграции фаззинга в WebRTC сервер janus-gateway. Сам скрипт занимает где-то 100 строчек на bash. Конечно, это не так много, однако, как только проект начнёт меняться, эти скрипты тоже необходимо будет поддерживать в актуальном состоянии. На это может потребоваться дополнительное время.

Фаззинг в облаке

Наконец, мы добрались до финальной главы непосредственно про организацию continuous fuzzing. Сначала мы посмотрим, как с этой задачей справились в Google, а затем что получилось у нас.

Фаззинг-ферма от Google

В 2012 году компания Google анонсировала ClusterFuzz облачную инфраструктуру для поиска уязвимостей в компонентах своего браузера. В его описании приведена схема, наглядно показывающая, как должен выглядеть continuous fuzzing, интегрированный в процесс разработки.

Схема сontinious fuzzing от google

Пробежимся по элементам схемы:

• Developer команда разработчиков проекта
• Upstream project репозиторий с проектом
• Oss-fuzz вспомогательный репозиторий для интеграции continuous fuzzing. Хранит build-конфиги фаззеров
• Builder Сборщик фаззеров. Его задача скачивать исходники фаззеров из репозитория с проектом, подхватывать build-конфиги из oss-fuzz и производить сборку фаззеров. После сборки builder закачивает в хранилку (GCS bucket) файлы, необходимые для запуска фаззера
• Clusterfuzz масштабируемая система для управления процессом фаззинга. Отвечает за планирование запусков фаззеров, обработку полученных от них данных, сбор статистики и многое другое. Пополняет свою коллекцию фаззеров из хранилки.
• Issue tracker система отслеживания задач: youtrack, jira и подобные им программы. Сюда приходят отчёты о найденных уязвимостях

А теперь рассмотрим сам процесс интеграции. Начнём с того, что у команды разработчиков должен иметься репозиторий с кодом проекта. Здесь же должны храниться исходные коды фаззеров. Для интеграции необходимо подготовить build-конфиги для сборки и запуска фаззеров в инфраструктуре Google. Готовые build-конфиги нужно добавить в репозиторий oss-fuzz через пулл-реквест. После принятия пулл-реквеста можно радоваться: фаззеры будут автоматически собраны и запущены в Clusterfuzz. С каждым новым коммитом в oss-fuzz будут подхватываться фаззеры из репозитория с проектом. Если вдруг нашлась уязвимость, то уведомление придёт к разработчикам в issue tracker. Разработчики своевременно исправляют баги и все остаются довольными.

Почему решение от Google подходит не всем?

Отметим, что ферма для фаззинга от Google это круто. Впрочем, и в этой бочке мёда есть ложка дёгтя. Несмотря на то, что Clusterfuzz имеет открытый код, он жёстко привязан к облачным сервисам Google.

Это означает, что:

• Для закрытых проектов такая система не подходит.
• Изменить что-то под себя вряд ли получится.

Поэтому мы сделали свою фаззинг-ферму. Собственное решение для continuous fuzzing позволяет нам:

• Снять ограничения на используемую инфраструктуру. Ферму можно запускать как в публичном облаке, так и у себя локально.
• Полностью контролировать работу фермы: мониторинг, конфигурирование, добавление собственных фич и т.д.
• Осуществлять фаззинг проприетарного программного обеспечения.

Схема нашей фермы похожа на схему от Google, однако она проще. Основная логика сосредоточена в компоненте FuzzFarm. Он выполняет те же задачи, что и Clusterfuzz. Ферма использует только наши внутренние ресурсы и может работать без доступа к интернету.

Схема работы нашей фаззинг-фермы

Интерфейс пользователя мы сделали простым и минималистичным. При этом он покрывает все необходимые задачи в continuous fuzzing.

Интерфейс пользователя нашей фаззинг-фермы

Что получаем в итоге?

В итоге мы имеем следующие преимущества использования continuous fuzzing:

• Для разработчика это более быстрое обнаружение ошибок и готовые данные для юнит-тестов. В баг-трекер приходит уведомление, разработчик устраняет ошибку и быстро делает тест с использованием полученного от фермы кейса.
• Ещё один шаг к оптимизации заключается в автоматическом создании юнит-тестов из полученных результатов фаззинга. Тестировщик будет рад :)
• Наконец, можно добавить автоматический анализ крэшей. Для безопасника будет намного лучше, если система за него будет оценивать критичность найденных уязвимостей и предоставлять ему красивый отчёт. Более того, запуск фаззеров и обработка их результатов производятся автоматически. У безопасника появится время попить кофе.

Заключение

Тема фаззинга на сегодняшний день уже вышла далеко за пределы просто одного из видов тестирования. Это целое направление, объединившее единомышленников в коммьюнити. Уже проходят целые конференции и митапы, посвященные только фаззингу. Это большое и разноплановое направление со множеством перспектив развития, к примеру, Structure-Aware фаззинг, о котором мы постараемся рассказать позже. Поскольку, как вы могли заметить, развитие как отдельных инструментов, так и в целом направления не стоит на месте, существует возможность улучшать показатели по всем параметрам: скорости, качества, трудозатратам и т.п.

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

В конце хотелось бы поблагодарить за помощь в создании доклада всю нашу команду отдела Исследований и особенно других членов фаззинг-команды:

• Дмитрий Евдокимов d1g1
• Никита Кныжов presler

Соавтор статьи: Павел Князев poulix

Материалы

• 25 Years of Fuzzing
• Фаззинг важный этап безопасной разработки
• Зоопарк AFL фазеров
• Фаззинг глазами программиста
• The Art, Science, and Engineering of Fuzzing: A Survey
• Pulling JPEG out of thin airl
• AFL dictionaries
• Building Secure and Reliable Systems
• Fuzzing for Software Security Testing and Quality Assurance
• Recent Papers Related To Fuzzing

Порталы и мобильные приложения

Интеграция по стандартной схеме

Нестандартное в стандартном

Сложности проекта

Резюме

• внедряемое нами решение достаточно взрослое, чтобы проявлять нужную гибкость для построения процессов DevSecOps в кардинально разных средах внедрения. Гибкость достигается за счёт большого набора встроенных и кастомных интеграций, без которых трудозатраты на внедрение возросли бы в разы или сделали бы его невозможным;
• настройка нужной автоматизации и последующий разбор результатов не требуют необъятного количества трудозатрат даже при огромном скоупе работ. Согласование и построение внедряемых процессов и полная их автоматизация возможны усилиями небольшой экспертной группы из 3-4 человек;
• внедрение средств автоматической проверки кода и практик DevSecOps позволяет выявить недостатки текущих процессов DevOps и становится поводом для их настройки, улучшения, унификации и регламентирования. В конечном счёте получается win-win ситуация для всех участников процесса от рядовых разработчиков до топ-менеджеров отделов инженерии и ИБ.

Утилиты проверки безопасности

Варианты внедрения утилит

1. Проверка корректности и безопасности инструкций Dockerfile утилитой линтером Hadolint
2. Проверка корректности и безопасности конечного и промежуточных образов утилитой Dockle
3. Проверка наличия общеизвестных уязвимостей (CVE) в базовом образе и ряде зависимостей утилитой Trivy

Интеграция в GitLab CI/CD

sudo apt-get update && sudo apt-get install docker.io

sudo addgroup <username> docker

ip addr

docker run --detach \--hostname 192.168.1.112 \--publish 443:443 --publish 80:80 \--name gitlab \--restart always \--volume /srv/gitlab/config:/etc/gitlab \--volume /srv/gitlab/logs:/var/log/gitlab \--volume /srv/gitlab/data:/var/opt/gitlab \gitlab/gitlab-ce:latest

sudo curl -L --output /usr/local/bin/gitlab-runner https://gitlab-runner-downloads.s3.amazonaws.com/latest/binaries/gitlab-runner-linux-amd64

sudo chmod +x /usr/local/bin/gitlab-runner

sudo useradd --comment 'GitLab Runner' --create-home gitlab-runner --shell /bin/bashsudo gitlab-runner install --user=gitlab-runner --working-directory=/home/gitlab-runnersudo gitlab-runner start

local@osboxes:~$ sudo gitlab-runner install --user=gitlab-runner --working-directory=/home/gitlab-runnerRuntime platform arch=amd64 os=linux pid=8438 revision=0e5417a3 version=12.0.1local@osboxes:~$ sudo gitlab-runner startRuntime platform arch=amd64 os=linux pid=8518 revision=0e5417a3 version=12.0.1

sudo gitlab-runner register \--non-interactive \--url "http://<URL>/" \--registration-token "<Registration Token>" \--executor "docker" \--docker-privileged \--docker-image alpine:latest \--description "docker-runner" \--tag-list "docker,privileged" \--run-untagged="true" \--locked="false" \--access-level="not_protected"

FROM amd64/node:10.16.0-alpine@sha256:f59303fb3248e5d992586c76cc83e1d3700f641cbcd7c0067bc7ad5bb2e5b489 AS tsbuildCOPY package.json .COPY yarn.lock .RUN yarn installCOPY lib libCOPY tsconfig.json tsconfig.jsonCOPY tsconfig.app.json tsconfig.app.jsonRUN yarn buildFROM amd64/ubuntu:18.04@sha256:eb70667a801686f914408558660da753cde27192cd036148e58258819b927395LABEL maintainer="Rhys Arkins <rhys@arkins.net>"LABEL name="renovate"...COPY php.ini /usr/local/etc/php/php.iniRUN cp -a /tmp/piik/* /var/www/html/RUN rm -rf /tmp/piwikRUN chown -R www-data /var/www/htmlADD piwik-cli-setup /piwik-cli-setupADD reset.php /var/www/html/## ENTRYPOINT ##ADD entrypoint.sh /entrypoint.shENTRYPOINT ["/entrypoint.sh"]USER root

variables:    DOCKER_HOST: "tcp://docker:2375/"    DOCKERFILE: "mydockerfile.df" # name of the Dockerfile to analyse       DOCKERIMAGE: "bkimminich/juice-shop" # name of the Docker image to analyse    # DOCKERIMAGE: "knqyf263/cve-2018-11235" # test Docker image with several CRITICAL CVE    SHOWSTOPPER_PRIORITY: "CRITICAL" # what level of criticality will fail Trivy job    TRIVYCACHE: "$CI_PROJECT_DIR/.cache" # where to cache Trivy database of vulnerabilities for faster reuse    ARTIFACT_FOLDER: "$CI_PROJECT_DIR" services:    - docker:dind # to be able to build docker images inside the Runner stages:    - scan    - report    - publish HadoLint:    # Basic lint analysis of Dockerfile instructions    stage: scan    image: docker:git     after_script:    - cat $ARTIFACT_FOLDER/hadolint_results.json     script:    - export VERSION=$(wget -q -O - https://api.github.com/repos/hadolint/hadolint/releases/latest | grep '"tag_name":' | sed -E 's/.*"v([^"]+)".*/\1/')    - wget https://github.com/hadolint/hadolint/releases/download/v${VERSION}/hadolint-Linux-x86_64 && chmod +x hadolint-Linux-x86_64         # NB: hadolint will always exit with 0 exit code    - ./hadolint-Linux-x86_64 -f json $DOCKERFILE > $ARTIFACT_FOLDER/hadolint_results.json || exit 0     artifacts:        when: always # return artifacts even after job failure               paths:        - $ARTIFACT_FOLDER/hadolint_results.json Dockle:    # Analysing best practices about docker image (users permissions, instructions followed when image was built, etc.)    stage: scan       image: docker:git     after_script:    - cat $ARTIFACT_FOLDER/dockle_results.json     script:    - export VERSION=$(wget -q -O - https://api.github.com/repos/goodwithtech/dockle/releases/latest | grep '"tag_name":' | sed -E 's/.*"v([^"]+)".*/\1/')    - wget https://github.com/goodwithtech/dockle/releases/download/v${VERSION}/dockle_${VERSION}_Linux-64bit.tar.gz && tar zxf dockle_${VERSION}_Linux-64bit.tar.gz    - ./dockle --exit-code 1 -f json --output $ARTIFACT_FOLDER/dockle_results.json $DOCKERIMAGE            artifacts:        when: always # return artifacts even after job failure               paths:        - $ARTIFACT_FOLDER/dockle_results.json Trivy:    # Analysing docker image and package dependencies against several CVE bases    stage: scan       image: docker:git     script:    # getting the latest Trivy    - apk add rpm    - export VERSION=$(wget -q -O - https://api.github.com/repos/knqyf263/trivy/releases/latest | grep '"tag_name":' | sed -E 's/.*"v([^"]+)".*/\1/')    - wget https://github.com/knqyf263/trivy/releases/download/v${VERSION}/trivy_${VERSION}_Linux-64bit.tar.gz && tar zxf trivy_${VERSION}_Linux-64bit.tar.gz         # displaying all vulnerabilities w/o failing the build    - ./trivy -d --cache-dir $TRIVYCACHE -f json -o $ARTIFACT_FOLDER/trivy_results.json --exit-code 0 $DOCKERIMAGE            # write vulnerabilities info to stdout in human readable format (reading pure json is not fun, eh?). You can remove this if you don't need this.    - ./trivy -d --cache-dir $TRIVYCACHE --exit-code 0 $DOCKERIMAGE         # failing the build if the SHOWSTOPPER priority is found    - ./trivy -d --cache-dir $TRIVYCACHE --exit-code 1 --severity $SHOWSTOPPER_PRIORITY --quiet $DOCKERIMAGE             artifacts:        when: always # return artifacts even after job failure        paths:        - $ARTIFACT_FOLDER/trivy_results.json     cache:        paths:        - .cache Report:    # combining tools outputs into one HTML    stage: report    when: always    image: python:3.5         script:    - mkdir json    - cp $ARTIFACT_FOLDER/*.json ./json/    - pip install json2html    - wget https://raw.githubusercontent.com/shad0wrunner/docker_cicd/master/convert_json_results.py    - python ./convert_json_results.py         artifacts:        paths:        - results.html

NB: Trivy требует для своего запуска установленные rpm и git. В противном случае он будет выдавать ошибки при сканировании RedHat-based образов и получении обновлений базы уязвимостей.

Shell-скрипт

~/docker_cicd$ ./docker_sec_check.sh[+] Setting environment variables[+] Installing required packages[+] Preparing necessary directories[+] Fetching sample Dockerfile2020-10-20 10:40:00 (45.3 MB/s) - Dockerfile saved [8071/8071][+] Pulling image to scanlatest: Pulling from bkimminich/juice-shop[+] Running Hadolint...Dockerfile:205 DL3015 Avoid additional packages by specifying `--no-install-recommends`Dockerfile:248 DL3002 Last USER should not be root...[+] Running Dockle...WARN    - DKL-DI-0006: Avoid latest tag        * Avoid 'latest' tagINFO    - CIS-DI-0005: Enable Content trust for Docker        * export DOCKER_CONTENT_TRUST=1 before docker pull/build...[+] Running Trivyjuice-shop/frontend/package-lock.json=====================================Total: 3 (UNKNOWN: 0, LOW: 1, MEDIUM: 0, HIGH: 2, CRITICAL: 0)+---------------------+------------------+----------+---------+-------------------------+|       LIBRARY       | VULNERABILITY ID | SEVERITY | VERSION |             TITLE       |+---------------------+------------------+----------+---------+-------------------------+| object-path         | CVE-2020-15256   | HIGH     | 0.11.4  | Prototype pollution in  ||                     |                  |          |         | object-path             |+---------------------+------------------+          +---------+-------------------------+| tree-kill           | CVE-2019-15599   |          | 1.2.2   | Code Injection          |+---------------------+------------------+----------+---------+-------------------------+| webpack-subresource | CVE-2020-15262   | LOW      | 1.4.1   | Unprotected dynamically ||                     |                  |          |         | loaded chunks           |+---------------------+------------------+----------+---------+-------------------------+juice-shop/package-lock.json============================Total: 20 (UNKNOWN: 0, LOW: 1, MEDIUM: 6, HIGH: 8, CRITICAL: 5)...juice-shop/package-lock.json============================Total: 5 (CRITICAL: 5)...[+] Removing left-overs[+] Making the output look pretty[+] Converting JSON results[+] Writing results HTML[+] Clean exit ============================================================[+] Everything is done. Find the resulting HTML report in results.html

Docker-образ со всеми утилитами

docker build -t dscan:image -f docker_security.df .

docker run --rm -v $(pwd)/results:/results -v $(pwd)/docker_security.df:/Dockerfile -e DOCKERIMAGE="bkimminich/juice-shop" dscan:image

[+] Setting environment variables[+] Running Hadolint/Dockerfile:3 DL3006 Always tag the version of an image explicitly[+] Running DockleWARN    - DKL-DI-0006: Avoid latest tag        * Avoid 'latest' tagINFO    - CIS-DI-0005: Enable Content trust for Docker        * export DOCKER_CONTENT_TRUST=1 before docker pull/buildINFO    - CIS-DI-0006: Add HEALTHCHECK instruction to the container image        * not found HEALTHCHECK statementINFO    - DKL-LI-0003: Only put necessary files        * unnecessary file : juice-shop/node_modules/sqlite3/Dockerfile        * unnecessary file : juice-shop/node_modules/sqlite3/tools/docker/architecture/linux-arm64/Dockerfile        * unnecessary file : juice-shop/node_modules/sqlite3/tools/docker/architecture/linux-arm/Dockerfile[+] Running Trivy...juice-shop/package-lock.json============================Total: 20 (UNKNOWN: 0, LOW: 1, MEDIUM: 6, HIGH: 8, CRITICAL: 5)...[+] Making the output look pretty[+] Starting the main module ============================================================[+] Converting JSON results[+] Writing results HTML[+] Clean exit ============================================================[+] Everything is done. Find the resulting HTML report in results.html

Результаты

О проекте

GK

Решение для интеграции

Стек технологий

Учитываем пожелания

Интеграция с GitLab

Интеграция с Active Directory

Резюме

Lessons to Be Learned

1. Учитываем регламенты и бюрократию

2. Текучка существенный тормоз процесса

3. Погружение в специфику избавит от множества проблем

Изменения в продукте по результатам проекта

1. Интеграция с AD

2. Jira

3. Изменение архитектуры приложения

Резюме

Один из наших читателей обратил наше внимание на Espressif IoT Development Framework. Он нашёл ошибку в коде проекта и поинтересовался, смог бы её найти статический анализатор PVS-Studio. Именно эту ошибку анализатор пока найти не может, зато нашёл множество других. По мотивам этой истории и найденных ошибок, мы решили написать классическую статью про проверку открытого проекта. Приятного изучения того, из-за чего IoT устройства могут "выстрелить вам в ногу".

Программно-аппаратные системы

Отец языка C++ Бьярне Страуструп как-то сказал:

"Си" позволяет очень просто выстрелить себе в ногу. На "Си++" сделать это сложнее, но, когда вы это делаете, отрывает всю ногу.

В нашем случае, заявление начинает приобретать несколько иной смысл. Из идеи, что программист может ошибиться и программа будет функционировать как-то неправильно, мы подходим к ситуациям, когда это может причинять реальный физический вред.

Такие проекты, как Espressif IoT Development Framework, служат для реализации программно-аппаратных систем, взаимодействующих с человеком и управляющие объектами в реальном мире. Всё это накладывает дополнительные требования к качеству и надёжности программного кода. Именно отсюда берут основы такие стандарты как MISRA или AUTOSAR. Впрочем, это уже другая тема.

Вернёмся к Espressif IoT Development Framework (исходный код на сайте GitHub: esp-idf). Вот его краткое описание:

ESP-IDF is Espressif's official IoT Development Framework for the ESP32 and ESP32-S series of SoCs. It provides a self-sufficient SDK for any generic application development on these platforms, using programming languages such as C and C++. ESP-IDF currently powers millions of devices in the field, and enables building a variety of network-connected products, ranging from simple light bulbs and toys to big appliances and industrial devices.

Думаю, читателям интересно посмотреть, уделяется ли разработчиками этого проекта достаточно внимания к качеству и надёжности. К сожалению, такой уверенности, нет. Познакомившись со статьей и описанием замеченных дефектов, вы поймете мои опасения. Итак, заваривайте чай/кофе, вас ждёт много текста и кода.

Предыстория

Ещё хочется рассказать, как появилась эта статья. Мне написал Юрий Попов (Hardcore IoT fullstack dev & CTO), который с интересом следит за нашими публикациями. Незадолго до этого он самостоятельно вручную нашёл ошибку в Espressif IoT Development Framework и поинтересовался, может ли выявить этот дефект PVS-Studio. Ошибка связана с опечаткой коде, а PVS-Studio всегда славился тем, что хорошо выявляет подобные ошибки.

Некорректный код находился в файле mdns.c:

mdns_txt_linked_item_t * txt = service->txt;while (txt) {  data_len += 2 + strlen(service->txt->key) + strlen(service->txt->value);  txt = txt->next;}

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

Правильный код:

data_len += 2 + strlen(txt->key) + strlen(txt->value);

К обоюдному разочарованию меня и читателя Юры, PVS-Studio не смог заметить эту ошибку. Он просто не знает про такой паттерн ошибки. Собственно, и наша команда не знала про такой паттерн. PVS-Studio, как и любой другой анализатор, умеет замечать только то, на что его запрограммировали :).

Что ж, жаль, но не страшно. Это один из источников, где можно черпать идеи по развитию PVS-Studio. Пользователи и клиенты присылают различные паттерны ошибок, которые они выявили в коде своих проектов, но про которые не знает PVS-Studio. И мы постепенно создаём новые диагностические правила. Так произойдёт и с рассмотренным выше паттерном. Мы уже выписали этот пример в TODO и реализуем новое диагностическое правило для выявления схожих ситуаций в одной из следующих версий анализатора.

По итогам всего этого, Юра сам написал небольшую заметку про эту ошибку, как он её искал и про PVS-Studio: "Баг в ESP-IDF: MDNS, Wireshark и при чём тут единороги". Плюс он уведомил авторов проекта о найденной ошибке: Spurious MDNS collision detection (IDFGH-4263).

На этом история не закончилось. Юра предложил нашей команде проверить проект и написать заметку о результатах. Мы не стали отказываться, так как весьма часто делаем подобные публикации для популяризации методологии статического анализа кода и заодно инструмента PVS-Studio :).

Правда проверку мы повели достаточно неуклюже. К сожалению, нет примера "собрать всё". Ну или мы не разобрались. Мы начали с getting_started\hello_world. Вроде бы он использует часть фреймворка, но не полностью. Так что можно найти и другие ошибки, добившись компиляции большего количества файлов фреймворка. Другими словами, то, что в статье будет описана только 71 ошибка, это наша недоработка :).

Надо понимать, что у меня не было задачи найти как можно больше ошибок. Поэтому, когда я бегло пробежался по неполному отчёту, я сразу понял, что материала и так более чем достаточно для статьи. Поэтому я поленился углубляться дальнейшее изучение проекта.

К счастью, у Юрия Попова, который заварил всю эту кашу, гораздо больше энтузиазма, чем у меня. Он сообщил, что смог добиться более полной компиляции фреймворка и проверил гораздо больше файлов. И, очень вероятно, вслед за моей статьей, выйдет его статья, где он рассмотрит дополнительную порцию ошибок.

Примеры, откуда берутся ложные/бессмысленные срабатывания

Всех исследователей, которые захотят проверить Espressif IoT Development Framework, я хочу предупредить, что понадобится предварительная настройка анализатора. Без неё вы утоните в большом количестве ложных/бесполезных срабатываний. Но анализатор не виноват.

В коде проекта очень активно используются директивы условной компиляции (#ifdef) и макросы. Такой стиль кодирования запутывает анализатор и порождает множество однотипных бесполезных предупреждений. Чтобы было понятнее, как и почему это происходит, рассмотрим пару примеров.

Предупреждение PVS-Studio: V547 Expression 'ret != 0' is always true. esp_hidd.c 45

esp_err_t esp_hidd_dev_init(....){  esp_err_t ret = ESP_OK;  ....  switch (transport) {#if CONFIG_GATTS_ENABLE  case ESP_HID_TRANSPORT_BLE:    ret = esp_ble_hidd_dev_init(dev, config, callback);    break;#endif /* CONFIG_GATTS_ENABLE */  default:    ret = ESP_FAIL;    break;  }  if (ret != ESP_OK) {    free(dev);    return ret;  }  ....}

Выбран такой режим компиляции, при котором макрос CONFIG_GATTS_ENABLE не объявлен. Поэтому, для анализатора этот код выглядит так:

esp_err_t ret = ESP_OK;....switch (transport) {default:  ret = ESP_FAIL;  break;}if (ret != ESP_OK) {

Анализатор вроде прав, что условие всегда истинно. С другой стороны, пользы от этого предупреждения нет, так как мы понимаем, что код совершенно корректен и имеет смысл. Подобные ситуации встречаются крайне часто, что затрудняет просмотр отчёта. Это такая неприятная плата за активное использование условной компиляции :).

Рассмотрим другой пример. В коде активно используется своя разновидность assert-макросов. К сожалению, они тоже сбивают анализатор с толку. Предупреждение PVS-Studio: V547 Expression 'sntp_pcb != NULL' is always true. sntp.c 664

#define LWIP_PLATFORM_ASSERT(x) do \  {printf("Assertion \"%s\" failed at line %d in %s\n", \    x, __LINE__, __FILE__); fflush(NULL); abort();} while(0)#ifndef LWIP_NOASSERT#define LWIP_ASSERT(message, assertion) do { if (!(assertion)) { \  LWIP_PLATFORM_ASSERT(message); }} while(0)#else  /* LWIP_NOASSERT */#define LWIP_ASSERT(message, assertion)#endif /* LWIP_NOASSERT */sntp_pcb = udp_new_ip_type(IPADDR_TYPE_ANY);LWIP_ASSERT("Failed to allocate udp pcb for sntp client", sntp_pcb != NULL);if (sntp_pcb != NULL) {

Анализатор видит, что код в которой раскрывается LWIP_ASSERT остановит выполнение программы (см. вызов функции abort), если указатель sntp_pcb будет нулевой. Поэтому PVS-Studio предупреждает, что следующая проверка (sntp_pcb != NULL) не имеет смысла.

С одной стороны, анализатор прав. Но всё поменяется, если макрос в другом режиме компиляции будет раскрыт в "ничто". В этом случае проверка уже будет иметь смысл. Да, при втором сценарии анализатор ругаться не будет, но сути это уже не меняет. В первом то случае у нас лишнее предупреждение.

Впрочем, всё это не страшно. Если взяться за проект всерьез, то можно настроить анализатор так, что большинство бессмысленных сообщений исчезнет. Ещё в ряде мест можно улучшить ситуацию, изменив стиль написания кода и макросов. Но это уже выходит за рамки данной статьи. Дополнительно можно использовать механизм подавления предупреждений в конкретных местах, в макросах и т.д. Есть ещё и механизм массовой разметки. Про всё это подробнее можно почитать в статье "Как внедрить статический анализатор кода в legacy проект и не демотивировать команду".

Security

Начнём с предупреждений, которые, на мой взгляд, связаны с темой security. Разработчики операционных систем, фреймворков и других подобных проектов, должны с особенным вниманием относиться к поиску слабостей кода, которые потенциально могут приводить к возникновению уязвимостей.

Для удобства классификации слабостей кода можно использовать CWE (Common Weakness Enumeration). В PVS-Studio можно включить отображение CWE ID для предупреждений. Для предупреждений этой главы я дополнительно приведу соответствующий CWE ID.

Подробнее тема поиска потенциальных уязвимостей раскрыта в статье "Статический анализатор кода PVS-Studio как защита от уязвимостей нулевого дня".

Ошибка N1; Порядок аргументов

Предупреждение PVS-Studio: V764 Possible incorrect order of arguments passed to 'crypto_generichash_blake2b__init_salt_personal' function: 'salt' and 'personal'. blake2b-ref.c 457

int blake2b_init_salt_personal(blake2b_state *S, const uint8_t outlen,                               const void *personal, const void *salt);intblake2b_salt_personal(uint8_t *out, const void *in, const void *key,                      const uint8_t outlen, const uint64_t inlen,                      uint8_t keylen, const void *salt, const void *personal){  ....  if (blake2b_init_salt_personal(S, outlen, salt, personal) < 0)    abort();  ....}

При вызове функции blake2b_init_salt_personal перепутаны местами аргументы personal и salt. Мне кажется, вряд ли это задумано специально и, скорее всего, это ошибка, возникшая по невнимательности. Я не ориентируюсь в коде проекта и в криптографии, но что-то мне подсказывает, что такая путаница может иметь нехорошие последствия.

Согласно CWE эта ошибка классифицируется как CWE-683: Function Call With Incorrect Order of Arguments.

Ошибка N2; Отбрасывание значащих бит

Предупреждение PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. mbc_tcp_master.c 387

static esp_err_t mbc_tcp_master_set_request(  char* name, mb_param_mode_t mode, mb_param_request_t* request,  mb_parameter_descriptor_t* reg_data){  ....  // Compare the name of parameter with parameter key from table  uint8_t comp_result = memcmp((const char*)name,                               (const char*)reg_ptr->param_key,                               (size_t)param_key_len);  if (comp_result == 0) {  ....}

Сохранять результат работы функции memcmp в однобайтовую переменную это очень плохо. Это дефект, который вполне может превратиться в реальную уязвимость, подобную этой: CVE-2012-2122. Подробнее, почему так писать нельзя, описано в документации к диагностике V642.

Если совсем кратко, то некоторые реализации функция memset могут возвращать в случае несовпадения блоков памяти не только значения 1 или -1. Функция, например, может вернуть значение 1024. А это число, записанное в переменную типа uint8_t превратится в 0.

Согласно CWE эта ошибка классифицируется как CWE-197: Numeric Truncation Error.

Ошибка N3 N20; Приватные данные остаются в памяти

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'prk' buffer. The memset_s() function should be used to erase the private data. dpp.c 854

#ifndef os_memset#define os_memset(s, c, n) memset(s, c, n)#endifstatic int dpp_derive_k1(const u8 *Mx, size_t Mx_len, u8 *k1,       unsigned int hash_len){  u8 salt[DPP_MAX_HASH_LEN], prk[DPP_MAX_HASH_LEN];  const char *info = "first intermediate key";  int res;  /* k1 = HKDF(<>, "first intermediate key", M.x) */  /* HKDF-Extract(<>, M.x) */  os_memset(salt, 0, hash_len);  if (dpp_hmac(hash_len, salt, hash_len, Mx, Mx_len, prk) < 0)    return -1;  wpa_hexdump_key(MSG_DEBUG, "DPP: PRK = HKDF-Extract(<>, IKM=M.x)",      prk, hash_len);  /* HKDF-Expand(PRK, info, L) */  res = dpp_hkdf_expand(hash_len, prk, hash_len, info, k1, hash_len);  os_memset(prk, 0, hash_len);             // <=  if (res < 0)    return -1;  wpa_hexdump_key(MSG_DEBUG, "DPP: k1 = HKDF-Expand(PRK, info, L)",                  k1, hash_len);  return 0;}

Очень распространённая ошибка. Компилятор вправе в целях оптимизации удалить вызов функции memset, так как после заполнения буфера нулями, он больше не используется. В результате приватные данные на самом деле не затираются, а продолжат болтаться где-то в памяти. Подробности можно узнать в статье "Безопасная очистка приватных данных".

Согласно CWE эта ошибка классифицируется как CWE-14: Compiler Removal of Code to Clear Buffers.

Другие ошибки этого типа:

• V597 The compiler could delete the 'memset' function call, which is used to flush 'prk' buffer. The memset_s() function should be used to erase the private data. dpp.c 883
• V597 The compiler could delete the 'memset' function call, which is used to flush 'prk' buffer. The memset_s() function should be used to erase the private data. dpp.c 942
• V597 The compiler could delete the 'memset' function call, which is used to flush 'psk' buffer. The memset_s() function should be used to erase the private data. dpp.c 3939
• V597 The compiler could delete the 'memset' function call, which is used to flush 'prk' buffer. The memset_s() function should be used to erase the private data. dpp.c 5729
• V597 The compiler could delete the 'memset' function call, which is used to flush 'Nx' buffer. The memset_s() function should be used to erase the private data. dpp.c 5934
• V597 The compiler could delete the 'memset' function call, which is used to flush 'val' buffer. The memset_s() function should be used to erase the private data. sae.c 155
• V597 The compiler could delete the 'memset' function call, which is used to flush 'keyseed' buffer. The memset_s() function should be used to erase the private data. sae.c 834
• V597 The compiler could delete the 'memset' function call, which is used to flush 'keys' buffer. The memset_s() function should be used to erase the private data. sae.c 838
• V597 The compiler could delete the 'memset' function call, which is used to flush 'pkey' buffer. The memset_s() function should be used to erase the private data. des-internal.c 422
• V597 The compiler could delete the 'memset' function call, which is used to flush 'ek' buffer. The memset_s() function should be used to erase the private data. des-internal.c 423
• V597 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The memset_s() function should be used to erase the private data. sha1-internal.c 358
• V597 The compiler could delete the 'memset' function call, which is used to flush 'A_MD5' buffer. The memset_s() function should be used to erase the private data. sha1-tlsprf.c 95
• V597 The compiler could delete the 'memset' function call, which is used to flush 'P_MD5' buffer. The memset_s() function should be used to erase the private data. sha1-tlsprf.c 96
• V597 The compiler could delete the 'memset' function call, which is used to flush 'A_SHA1' buffer. The memset_s() function should be used to erase the private data. sha1-tlsprf.c 97
• V597 The compiler could delete the 'memset' function call, which is used to flush 'P_SHA1' buffer. The memset_s() function should be used to erase the private data. sha1-tlsprf.c 98
• V597 The compiler could delete the 'memset' function call, which is used to flush 'T' buffer. The memset_s() function should be used to erase the private data. sha256-kdf.c 85
• V597 The compiler could delete the 'memset' function call, which is used to flush 'hash' buffer. The memset_s() function should be used to erase the private data. sha256-prf.c 105

Ошибка N21; Не удаляется буфер с приватными данными

Предупреждение PVS-Studio: V575 The null pointer is passed into 'free' function. Inspect the first argument. sae.c 1185

static int sae_parse_password_identifier(struct sae_data *sae,           const u8 *pos, const u8 *end){  wpa_hexdump(MSG_DEBUG, "SAE: Possible elements at the end of the frame",        pos, end - pos);  if (!sae_is_password_id_elem(pos, end)) {    if (sae->tmp->pw_id) {      wpa_printf(MSG_DEBUG,           "SAE: No Password Identifier included, but expected one (%s)",           sae->tmp->pw_id);      return WLAN_STATUS_UNKNOWN_PASSWORD_IDENTIFIER;    }    os_free(sae->tmp->pw_id);    sae->tmp->pw_id = NULL;    return WLAN_STATUS_SUCCESS; /* No Password Identifier */  }  ....}

Если с паролем что-то не так и указатель pw_id не нулевой, то выводится отладочное предупреждение и функция завершает свою работу. Что интересно, далее происходит попытка освободить буфер, используя нулевой указатель. Более того, в нулевой указатель вновь записывается NULL. Всё это не имеет смысла. Скорее всего, строчки освобождения памяти находятся не на своём месте. И мне кажется, код должен быть таким:

if (!sae_is_password_id_elem(pos, end)) {  if (sae->tmp->pw_id) {    wpa_printf(MSG_DEBUG,         "SAE: No Password Identifier included, but expected one (%s)",         sae->tmp->pw_id);    os_free(sae->tmp->pw_id);    sae->tmp->pw_id = NULL;    return WLAN_STATUS_UNKNOWN_PASSWORD_IDENTIFIER;  }  return WLAN_STATUS_SUCCESS; /* No Password Identifier */}

Во-первых, наверное, это устранит утечку памяти. Во-вторых, приватные данные перестанут где-то зря храниться длительное время в памяти.

Согласно CWE, эта формально ошибка классифицируется как CWE-628: Function Call with Incorrectly Specified Arguments. Так её классифицирует PVS-Studio, но, по сути и последствиям, это какая-то другая слабость кода.

Ошибка N22, N23; Использование неинициализированного буфера в качестве ключа

Предупреждение PVS-Studio: V614 Uninitialized buffer 'hex' used. Consider checking the second actual argument of the 'memcpy' function. wps_registrar.c 1657

int wps_build_cred(struct wps_data *wps, struct wpabuf *msg){  ....  } else if (wps->use_psk_key && wps->wps->psk_set) {    char hex[65];    wpa_printf(MSG_DEBUG,  "WPS: Use PSK format for Network Key");    os_memcpy(wps->cred.key, hex, 32 * 2);    wps->cred.key_len = 32 * 2;  } else if (wps->wps->network_key) {  ....}

Неинициализированный буфер hex используется для инициализации какого-то ключа. Зачем так сделано непонятно. Возможно, это попытка заполнить ключ какими-то случайным значением, но всё равно это очень плохой способ.

В любом случае этот код нуждается во внимательной проверке.

Согласно CWE, эта ошибка классифицируется как CWE-457: Use of Uninitialized Variable.

Аналогичная ошибка: V614 Uninitialized buffer 'hex' used. Consider checking the second actual argument of the 'memcpy' function. wps_registrar.c 1678

Опечатки и Copy-Paste

Ошибка N24; Copy-Paste классический

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. timer.c 292

esp_err_t timer_isr_register(....){  ....  if ((intr_alloc_flags & ESP_INTR_FLAG_EDGE) == 0) {    intr_source = ETS_TG1_T0_LEVEL_INTR_SOURCE + timer_num;  } else {    intr_source = ETS_TG1_T0_LEVEL_INTR_SOURCE + timer_num;  }  ....}

Есть подозрение, что строчку скопировали, но забыли что-то в ней изменить. В результате, независимо от условия, в переменную intr_source записывается одно и тоже значение.

Примечание. Бывает конечно, что так и задумано. Например, если пока, значения действительно должны совпадать (т.е. это "todo-код"). Но тогда такой код явно стоит снабдить поясняющим комментарием.

Ошибка N25; Не там поставлена скобка

Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. esp_tls_mbedtls.c 446

esp_err_t set_client_config(....){ .... if ((ret = mbedtls_ssl_conf_alpn_protocols(&tls->conf, cfg->alpn_protos) != 0)) {   ESP_LOGE(TAG, "mbedtls_ssl_conf_alpn_protocols returned -0x%x", -ret);   ESP_INT_EVENT_TRACKER_CAPTURE(tls->error_handle, ERR_TYPE_MBEDTLS, -ret);   return ESP_ERR_MBEDTLS_SSL_CONF_ALPN_PROTOCOLS_FAILED; } ....}

Приоритет оператора сравнения выше, чем приоритет оператора присваивания. Поэтому условие вычисляется следующим образом:

TEMP = mbedtls_ssl_conf_alpn_protocols(....) != 0;if ((ret = TEMP))  PRINT(...., -ret);

В принципе, ошибочная ситуация поймается и обработается в коде, но не так, как задумано. Предполагалось распечатывать статус ошибки, который хранится в переменной ret. Но значение ret всегда будет равно 0 или 1. Поэтому, если что-то пойдёт не так, всегда будет распечатываться только одно значение (-1).

Ошибка возникла из-за того, что не там поставлена скобочка. Правильный код:

if ((ret = mbedtls_ssl_conf_alpn_protocols(&tls->conf, cfg->alpn_protos)) != 0)

Теперь всё будет вычисляться как нужно:

ret = mbedtls_ssl_conf_alpn_protocols(....);if (ret != 0)  PRINT(...., -ret);

Рассмотрим ещё один очень похожий случай.

Ошибка N26; MP_MEM превращается в MP_YES

V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. libtommath.h 1660

В начале рассмотрим некоторые константы. Они пригодятся нам чуть ниже.

#define MP_OKAY       0   /* ok result */#define MP_MEM        -2  /* out of mem */#define MP_VAL        -3  /* invalid input */#define MP_YES        1   /* yes response */

Далее следует сказать, что существует функция mp_init_multi, которая может возвращать значения MP_OKAY и MP_MEM:

static int mp_init_multi(mp_int *mp, ...);

И теперь собственно код с ошибкой:

static intmp_div(mp_int * a, mp_int * b, mp_int * c, mp_int * d){  ....  /* init our temps */  if ((res = mp_init_multi(&ta, &tb, &tq, &q, NULL) != MP_OKAY)) {     return res;  }  ....}

Рассмотрим проверку более тщательно:

if ((res = mp_init_multi(....) != MP_OKAY))

Вновь не там поставлена скобка. Поэтому в начале вычисляется:

TEMP = (mp_init_multi(....) != MP_OKAY);

Значение TEMP может быть только 0 или 1. Этим числам соответствуют константы MB_OKAY и MP_YES.

Далее выполняется присваивание и одновременно проверка:

if ((res = TEMP))   return res;

Видите подвох? Статус ошибки MP_MEM (-2) вдруг превратился в статус MB_YES (1). Последствия предсказать не могу, но ничего хорошего в этом нет.

Ошибка N27; Забыли разыменовать указатель

Предупреждение PVS-Studio: V595 The 'outbuf' pointer was utilized before it was verified against nullptr. Check lines: 374, 381. protocomm.c 374

static int protocomm_version_handler(uint32_t session_id,                                     const uint8_t *inbuf, ssize_t inlen,                                     uint8_t **outbuf, ssize_t *outlen,                                     void *priv_data){    protocomm_t *pc = (protocomm_t *) priv_data;    if (!pc->ver) {        *outlen = 0;        *outbuf = NULL;                                  // <=        return ESP_OK;    }    /* Output is a non null terminated string with length specified */    *outlen = strlen(pc->ver);    *outbuf = malloc(*outlen);                           // <=    if (outbuf == NULL) {                                // <=        ESP_LOGE(TAG, "Failed to allocate memory for version response");        return ESP_ERR_NO_MEM;    }    memcpy(*outbuf, pc->ver, *outlen);    return ESP_OK;}

Сообщение анализатора, на первый взгляд, может показаться непонятным. Давайте разбираться.

Если указатель pc->ver является нулевым, то функция досрочно завершает свою работу и при этом записывает значение по адресу, хранящегося в указателе outbuf:

*outbuf = NULL;

Запись по этому адресу происходит и далее:

*outbuf = malloc(*outlen);

А не нравится анализатору то, что затем этот указатель проверяется:

if (outbuf == NULL)

Действительно, это неправильно, в начале разыменовывать указатель, а только потом его проверять. Ляп в том, что на самом то деле должны были проверять не сам указатель, а то, что в него записали. Здесь просто в проверке опечатались и пропустили оператор разыменования (*).

Правильный код:

*outbuf = malloc(*outlen);if (*outbuf == NULL) {  ESP_LOGE(TAG, "Failed to allocate memory for version response");  return ESP_ERR_NO_MEM;}

Ошибка N28; Повторное присваивание

Предупреждение PVS-Studio: V519 The 'usRegCount' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 186, 187. mbfuncholding.c 187

eMBExceptioneMBFuncReadHoldingRegister( UCHAR * pucFrame, USHORT * usLen ){  ....  USHORT          usRegCount;  ....  usRegCount = ( USHORT )( pucFrame[MB_PDU_FUNC_READ_REGCNT_OFF] << 8 );  usRegCount = ( USHORT )( pucFrame[MB_PDU_FUNC_READ_REGCNT_OFF + 1] );  ....}

Код явно писался методом Copy-Paste. Строчку скопировали, но изменили только частично. По соседству есть вот такой осмысленный код:

usRegCount = ( USHORT )( pucFrame[MB_PDU_FUNC_WRITE_MUL_REGCNT_OFF] << 8 );usRegCount |= ( USHORT )( pucFrame[MB_PDU_FUNC_WRITE_MUL_REGCNT_OFF + 1] );

Видимо, и в рассмотренном коде с ошибкой, следовало в первой строке использовать оператор =, а во второй оператор |=.

Логические ошибки

Ошибка N29 N31; Неправильная работа с кодами возврата (Rare)

Предупреждение PVS-Studio: V547 Expression is always false. linenoise.c 256

static int getColumns(void) {  ....  /* Restore position. */  if (cols > start) {    char seq[32];    snprintf(seq,32,"\x1b[%dD",cols-start);    if (fwrite(seq, 1, strlen(seq), stdout) == -1) {      /* Can't recover... */    }    flushWrite();  }  ....}

Это безобидный вариант неправильной обработки статуса, возвращаемого функцией. Ошибка безобидна в том смысле, что никакой обработки и не предусмотрено. Не получилось записать строчку, так не получилось :). Хотя, как я уже сказал, этот безобидный вариант, такой стиль написания программ явно не является образцом для подражания.

Суть же самой ошибки в том, что функция fwrite не возвращает статус -1. Это физически невозможно, так как функция fwrite возвращает значение целочисленного типа size_t:

size_t fwrite( const void *restrict buffer, size_t size, size_t count,               FILE *restrict stream );

А вот что возвращает эта функция:

The number of objects written successfully, which may be less than count if an error occurs.

If size or count is zero, fwrite returns zero and performs no other action.

Таким образом, проверка статуса является неверной.

Аналогичные места безобидной неправильной проверки статуса:

• V547 Expression is always false. linenoise.c 481
• V547 Expression is always false. linenoise.c 569

Ошибка N32, N33; Неправильная работа с кодами возврата (Medium)

Предупреждение PVS-Studio: V547 Expression is always false. linenoise.c 596

int linenoiseEditInsert(struct linenoiseState *l, char c) {  ....  if (fwrite(&c,1,1,stdout) == -1) return -1;  ....}

Хотя перед нами та же ошибка, что и в предыдущем случае, она более серьезна. Если не удаётся записать символ в файл, то функция linenoiseEditInsert должна прекратить свою работу и вернуть статус -1. Но этого не произойдёт, так как fwrite никогда не вернёт значение -1. Перед нами логическая ошибка обработки ситуации, когда не удаётся что-то записать в файл.

Аналогичную ошибку можно найти здесь: V547 Expression is always false. linenoise.c 742.

Ошибка N34; Неправильная работа с кодами возврата (Well Done)

Предупреждение PVS-Studio: V547 Expression is always false. linenoise.c 828

static int linenoiseEdit(char *buf, size_t buflen, const char *prompt)  ....  while(1) {    ....    if (fread(seq+2, 1, 1, stdin) == -1) break;    ....  }  ....}

Ошибка в том, что, как и в случае с fwrite, функция fread не возвращает в качестве статуса значение -1.

size_t fread( void *restrict buffer, size_t size, size_t count,              FILE *restrict stream );

Return value

Number of objects read successfully, which may be less than count if an error or end-of-file condition occurs.

If size or count is zero, fread returns zero and performs no other action.

fread does not distinguish between end-of-file and error, and callers must use feof and ferror to determine which occurred.

Этот код ещё более опасный. Ошибка чтения из файла не отлавливается, и программа продолжает работать с данными, случайно имеющимися в этот момент в буфере данных. Т.е. программа всегда считает, что она успешно прочитала из файла очередной байт, хотя это может быть и не так.

Ошибка N35; Использование оператора || там, где нужен оператор &&

Предупреждение PVS-Studio: V547 Expression is always true. essl_sdio.c 209

esp_err_t essl_sdio_init(void *arg, uint32_t wait_ms){  ....  // Set block sizes for functions 1 to given value (default value = 512).  if (ctx->block_size > 0 || ctx->block_size <= 2048) {    bs = ctx->block_size;  } else {    bs = 512;  }  ....}

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

Итак, перед нами всегда истинное условие. Ведь некая переменная всегда или больше 0 или меньше 2048. Из-за этого размер какого-то блока не будет ограничен значением 512.

Правильный вариант кода:

if (ctx->block_size > 0 && ctx->block_size <= 2048) {  bs = ctx->block_size;} else {  bs = 512;}

Ошибка N35 N38; Переменная не изменяется

Предупреждение PVS-Studio: V547 Expression 'depth <= 0' is always false. panic_handler.c 169

static void print_backtrace(const void *f, int core){  XtExcFrame *frame = (XtExcFrame *) f;  int depth = 100;                                          // <=  //Initialize stk_frame with first frame of stack  esp_backtrace_frame_t stk_frame =    {.pc = frame->pc, .sp = frame->a1, .next_pc = frame->a0};  panic_print_str("\r\nBacktrace:");  print_backtrace_entry(esp_cpu_process_stack_pc(stk_frame.pc),                        stk_frame.sp);  //Check if first frame is valid  bool corrupted =    !(esp_stack_ptr_is_sane(stk_frame.sp) &&      (esp_ptr_executable((void *)esp_cpu_process_stack_pc(stk_frame.pc)) ||       /* Ignore the first corrupted PC in case of InstrFetchProhibited */       frame->exccause == EXCCAUSE_INSTR_PROHIBITED));  //Account for stack frame that's already printed  uint32_t i = ((depth <= 0) ? INT32_MAX : depth) - 1;      // <=  ....}

Переменной depth присваивается значение 100, и до момента проверки этой переменной её значение нигде не изменяется. Это весьма подозрительно. Где-то что-то забыли сделать?

Аналогичные случаи:

• V547 Expression 'xAlreadyYielded == ((BaseType_t) 0)' is always true. event_groups.c 260
• V547 Expression 'xAlreadyYielded == ((BaseType_t) 0)' is always true. tasks.c 1475
• V547 Expression 'xAlreadyYielded == ((BaseType_t) 0)' is always true. tasks.c 1520

Ошибка N39; Использование неинициализированного буфера

Предупреждение PVS-Studio: V614 Potentially uninitialized buffer 'k' used. Consider checking the second actual argument of the 'sae_derive_keys' function. sae.c 854

int sae_process_commit(struct sae_data *sae){  u8 k[SAE_MAX_PRIME_LEN];  if (sae->tmp == NULL ||      (sae->tmp->ec && sae_derive_k_ecc(sae, k) < 0) ||      (sae->tmp->dh && sae_derive_k_ffc(sae, k) < 0) ||      sae_derive_keys(sae, k) < 0)    return ESP_FAIL;  return ESP_OK;}

Ошибка в логике. Предположим, что указатели ec и dh являются нулевыми. В этом случае массив k не инициализируется, но функция sae_derive_keys всё равно начнёт его обрабатывать.

Ошибка N40; Всегда ложное условие

Предупреждение PVS-Studio: V547 Expression 'bit_len == 32' is always false. spi_flash_ll.h 371

static inline void spi_flash_ll_set_usr_address(spi_dev_t *dev, uint32_t addr,                                                int bit_len){  // The blank region should be all ones  if (bit_len >= 32) {    dev->addr = addr;    dev->slv_wr_status = UINT32_MAX;  } else {    uint32_t padding_ones = (bit_len == 32? 0 : UINT32_MAX >> bit_len);    dev->addr = (addr << (32 - bit_len)) | padding_ones;  }}

Как легко увидеть, условие bit_len == 32 всегда даст ложный результат. Возможно, выше следовало написать не больше-или-равно (>=), а просто больше (>).

Ошибка N41; Повторное присваивание

Предупреждение PVS-Studio: V519 The '* pad_num' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 46, 48. touch_sensor_hal.c 48

void touch_hal_get_wakeup_status(touch_pad_t *pad_num){  uint32_t touch_mask = 0;  touch_ll_read_trigger_status_mask(&touch_mask);  if (touch_mask == 0) {    *pad_num = -1;  }  *pad_num = (touch_pad_t)(__builtin_ffs(touch_mask) - 1);}

Код явно ошибочен и возможно здесь не хватает оператора else. Я не уверен, но, возможно, код должен выглядеть так:

void touch_hal_get_wakeup_status(touch_pad_t *pad_num){  uint32_t touch_mask = 0;  touch_ll_read_trigger_status_mask(&touch_mask);  if (touch_mask == 0) {    *pad_num = -1;  } else {    *pad_num = (touch_pad_t)(__builtin_ffs(touch_mask) - 1);  }}

Выход за границу массива

Ошибка N42; Неправильная граничная проверка

Предупреждение PVS-Studio: V557 Array overrun is possible. The value of 'frame->exccause' index could reach 16. gdbstub_xtensa.c 132

int esp_gdbstub_get_signal(const esp_gdbstub_frame_t *frame){  const char exccause_to_signal[] =    {4, 31, 11, 11, 2, 6, 8, 0, 6, 7, 0, 0, 7, 7, 7, 7};  if (frame->exccause > sizeof(exccause_to_signal)) {    return 11;  }  return (int) exccause_to_signal[frame->exccause];}

Возможен выход за границу массива на 1 элемент. Для правильной проверки следует использовать не оператор больше, а больше-или-равно:

if (frame->exccause >= sizeof(exccause_to_signal)) {

Ошибка N43; Длинный пример ошибки :)

В рассматриваемой функции выход за границу массива может произойти в двух местах, поэтому и предупреждений анализатора сразу два:

• V557 Array overrun is possible. The value of 'other_if' index could reach 3. mdns.c 2206
• V557 Array overrun is possible. The '_mdns_announce_pcb' function processes value '[0..3]'. Inspect the first argument. Check lines: 1674, 2213. mdns.c 1674

Приготовьтесь, это будет сложный случай. Для начала взглянем на следующие именованные константы:

typedef enum mdns_if_internal {    MDNS_IF_STA = 0,    MDNS_IF_AP = 1,    MDNS_IF_ETH = 2,    MDNS_IF_MAX} mdns_if_t;

Обратите внимание, что значение константы MDNS_IF_MAX равно 3.

Теперь взглянем на определение структуры mdns_server_s. Здесь нам важно, что массив interfaces состоит из 3 элементов:

typedef struct mdns_server_s {    struct {        mdns_pcb_t pcbs[MDNS_IP_PROTOCOL_MAX];    } interfaces[MDNS_IF_MAX];    const char * hostname;    const char * instance;    mdns_srv_item_t * services;    SemaphoreHandle_t lock;    QueueHandle_t action_queue;    mdns_tx_packet_t * tx_queue_head;    mdns_search_once_t * search_once;    esp_timer_handle_t timer_handle;} mdns_server_t;mdns_server_t * _mdns_server = NULL;

Это ещё не всё. Нам понадобится заглянуть внутрь функции _mdns_get_other_if. Обратите внимание, что она может вернуть константу MDNS_IF_MAX. Т.е. она может вернуть значение 3.

static mdns_if_t _mdns_get_other_if (mdns_if_t tcpip_if){  if (tcpip_if == MDNS_IF_STA) {    return MDNS_IF_ETH;  } else if (tcpip_if == MDNS_IF_ETH) {     return MDNS_IF_STA;  }  return MDNS_IF_MAX;}

И вот, наконец, мы добрались до ошибок:

static void _mdns_dup_interface(mdns_if_t tcpip_if){    uint8_t i;    mdns_if_t other_if = _mdns_get_other_if (tcpip_if);    for (i=0; i<MDNS_IP_PROTOCOL_MAX; i++) {        if (_mdns_server->interfaces[other_if].pcbs[i].pcb) {        // <=            //stop this interface and mark as dup            if (_mdns_server->interfaces[tcpip_if].pcbs[i].pcb) {                _mdns_clear_pcb_tx_queue_head(tcpip_if, i);                _mdns_pcb_deinit(tcpip_if, i);            }            _mdns_server->interfaces[tcpip_if].pcbs[i].state = PCB_DUP;            _mdns_announce_pcb(other_if, i, NULL, 0, true);          // <=        }    }}

Итак, мы знаем, что функция _mdns_get_other_if может вернуть тройку. Значит переменная other_if может быть равна трём. И вот первый потенциальный выход за границу массива:

if (_mdns_server->interfaces[other_if].pcbs[i].pcb)

Второе место, где опасно используется переменная other_if, - это вызов функции _mdns_announce_pcb:

_mdns_announce_pcb(other_if, i, NULL, 0, true);

Заглянем в эту функцию:

static void _mdns_announce_pcb(mdns_if_t tcpip_if,                               mdns_ip_protocol_t ip_protocol,                               mdns_srv_item_t ** services,                               size_t len, bool include_ip){  mdns_pcb_t * _pcb = &_mdns_server->interfaces[tcpip_if].pcbs[ip_protocol];  ....}

Опять может использоваться индекс 3 для доступа к массиву, состоящего из 3 элементов. А максимальный доступный индекс это двойка.

Нулевые указатели

Ошибка N44 N47; Ошибка очерёдности проверки указателей

Предупреждение PVS-Studio: V595 The 'hapd->wpa_auth' pointer was utilized before it was verified against nullptr. Check lines: 106, 113. esp_hostap.c 106

bool hostap_deinit(void *data){  struct hostapd_data *hapd = (struct hostapd_data *)data;  if (hapd == NULL) {    return true;  }  if (hapd->wpa_auth->wpa_ie != NULL) {    os_free(hapd->wpa_auth->wpa_ie);  }  if (hapd->wpa_auth->group != NULL) {    os_free(hapd->wpa_auth->group);  }  if (hapd->wpa_auth != NULL) {    os_free(hapd->wpa_auth);  }  ....}

Неправильная последовательность проверки указателей:

if (hapd->wpa_auth->group != NULL)....if (hapd->wpa_auth != NULL)

Если указатель hapd->wpa_auth окажется нулевым, то всё плохо. Последовательность действий нужно поменять местами и сделать вложенной:

if (hapd->wpa_auth != NULL){  ....  if (hapd->wpa_auth->group != NULL)  ....}

Аналогичные ошибки:

• V595 The 'hapd->conf' pointer was utilized before it was verified against nullptr. Check lines: 118, 125. esp_hostap.c 118
• V595 The 'sm' pointer was utilized before it was verified against nullptr. Check lines: 1637, 1647. esp_wps.c 1637
• V595 The 'sm' pointer was utilized before it was verified against nullptr. Check lines: 1693, 1703. esp_wps.c 1693

Ошибка N48 N64; Нет проверки указателя после выделения памяти

В целом, в проекте принято проверять, удалось выделить память или нет. Т.е. много кода с подобными проверками:

dhcp_data = (struct dhcp *)malloc(sizeof(struct dhcp));if (dhcp_data == NULL) {  return ESP_ERR_NO_MEM;}

Но местами про проверки забыли.

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'exp'. Check lines: 3470, 3469. argtable3.c 3470

TRex *trex_compile(const TRexChar *pattern,const TRexChar **error,int flags){  TRex *exp = (TRex *)malloc(sizeof(TRex));  exp->_eol = exp->_bol = NULL;  exp->_p = pattern;  ....}

Этот вид ошибки сложнее и опаснее, чем может показаться на первый взгляд. Подробнее эта тема разбирается в статье "Почему важно проверять, что вернула функция malloc".

Другие места, где отсутствуют проверки:

• V522 There might be dereferencing of a potential null pointer 's_ledc_fade_rec[speed_mode][channel]'. Check lines: 668, 667. ledc.c 668
• V522 There might be dereferencing of a potential null pointer 'environ'. Check lines: 108, 107. syscall_table.c 108
• V522 There might be dereferencing of a potential null pointer 'it'. Check lines: 150, 149. partition.c 150
• V522 There might be dereferencing of a potential null pointer 'eth'. Check lines: 167, 159. wpa_auth.c 167
• V522 There might be dereferencing of a potential null pointer 'pt'. Check lines: 222, 219. crypto_mbedtls-ec.c 222
• V522 There might be dereferencing of a potential null pointer 'attr'. Check lines: 88, 73. wps.c 88
• V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 725, 724. coap_mbedtls.c 725
• V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 3504, 3503. argtable3.c 3504
• V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 496, 495. mqtt_client.c 496
• V575 The potential null pointer is passed into 'strcpy' function. Inspect the first argument. Check lines: 451, 450. transport_ws.c 451
• V769 The 'buffer' pointer in the 'buffer + n' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 186, 181. cbortojson.c 186
• V769 The 'buffer' pointer in the 'buffer + len' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 212, 207. cbortojson.c 212
• V769 The 'out' pointer in the 'out ++' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 233, 207. cbortojson.c 233
• V769 The 'parser->m_bufferPtr' pointer in the expression equals nullptr. The resulting value of arithmetic operations on this pointer is senseless and it should not be used. xmlparse.c 2090
• V769 The 'signature' pointer in the 'signature + curve->prime_len' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 4112, 4110. dpp.c 4112
• V769 The 'key' pointer in the 'key + 16' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 634, 628. eap_mschapv2.c 634

Ошибка N65, N66; Нет проверки указателя после выделения памяти (показательный случай)

Следующий код содержит точно такую же ошибку, как мы рассматривали выше, но она более показательная и яркая. Обратите внимания, что для выделения памяти используется функция realloc.

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'exp->_nodes' is lost. Consider assigning realloc() to a temporary pointer. argtable3.c 3008

static int trex_newnode(TRex *exp, TRexNodeType type){  TRexNode n;  int newid;  n.type = type;  n.next = n.right = n.left = -1;  if(type == OP_EXPR)    n.right = exp->_nsubexpr++;  if(exp->_nallocated < (exp->_nsize + 1)) {    exp->_nallocated *= 2;    exp->_nodes = (TRexNode *)realloc(exp->_nodes,                                      exp->_nallocated * sizeof(TRexNode));  }  exp->_nodes[exp->_nsize++] = n; // NOLINT(clang-analyzer-unix.Malloc)  newid = exp->_nsize - 1;  return (int)newid;}

Во-первых, если функция realloc вернёт NULL, то будет потеряно предыдущее значение указателя exp->_nodes. Возникнет утечка памяти.

Во-вторых, если функция realloc вернёт NULL, то запись значения произойдёт вовсе не по нулевому указателю. Имеется в виду эта строка:

exp->_nodes[exp->_nsize++] = n;

Значение exp->_nsize++ может быть любым, и, если запись произойдёт в какую-то случайную область памяти, доступную для записи, то программа продолжит своё выполнение, как ни в чём не бывало. При этом будут разрушены структуры данных, что приведёт к непредсказуемым последствиям.

Ещё одна такая ошибка: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_context->pki_sni_entry_list' is lost. Consider assigning realloc() to a temporary pointer. coap_mbedtls.c 737

Прочие ошибки

Ошибка N67; Лишний или неверный код

Предупреждение PVS-Studio: V547 Expression 'ret != 0' is always false. sdio_slave.c 394

esp_err_t sdio_slave_start(void){  ....  critical_exit_recv();  ret = ESP_OK;  if (ret != ESP_OK) return ret;  sdio_slave_hal_set_ioready(context.hal, true);  return ESP_OK;}

Это странный код, который можно сократить до:

esp_err_t sdio_slave_start(void){  ....  critical_exit_recv();  sdio_slave_hal_set_ioready(context.hal, true);  return ESP_OK;}

Есть здесь ошибка или нет, мне сказать сложно. Возможно, здесь написано совсем не то, что задумано. А возможно, этот код появился в процессе неудачного рефакторинга и, на самом деле, является корректным. В этом случае его действительно достаточно немного упростить, чтобы он смотрелся красивей и понятней. Одно точно этот код заслуживает внимания и проверки автором.

Ошибка N68; Лишний или неверный код

Предупреждение PVS-Studio: V547 Expression 'err != 0' is always false. sdio_slave_hal.c 96

static esp_err_t sdio_ringbuf_send(....){  uint8_t* get_ptr = ....;  esp_err_t err = ESP_OK;  if (copy_callback) {    (*copy_callback)(get_ptr, arg);  }  if (err != ESP_OK) return err;  buf->write_ptr = get_ptr;  return ESP_OK;}

В общем-то, всё то же самое, что и в предыдущем случае. Переменная err является лишней или её забыли изменить.

Ошибка N69; Использование потенциально неинициализированного буфера

Предупреждение PVS-Studio: V614 Potentially uninitialized buffer 'seq' used. Consider checking the first actual argument of the 'strlen' function. linenoise.c 435

void refreshShowHints(struct abuf *ab, struct linenoiseState *l, int plen) {    char seq[64];    if (hintsCallback && plen+l->len < l->cols) {        int color = -1, bold = 0;        char *hint = hintsCallback(l->buf,&color,&bold);        if (hint) {            int hintlen = strlen(hint);            int hintmaxlen = l->cols-(plen+l->len);            if (hintlen > hintmaxlen) hintlen = hintmaxlen;            if (bold == 1 && color == -1) color = 37;            if (color != -1 || bold != 0)                snprintf(seq,64,"\033[%d;%d;49m",bold,color);            abAppend(ab,seq,strlen(seq));                       // <=            abAppend(ab,hint,hintlen);            if (color != -1 || bold != 0)                abAppend(ab,"\033[0m",4);            /* Call the function to free the hint returned. */            if (freeHintsCallback) freeHintsCallback(hint);        }    }}

Буфер seq может быть заполнен, а может быть и не заполнен! Он заполняется только при выполнении условия:

if (color != -1 || bold != 0)  snprintf(seq,64,"\033[%d;%d;49m",bold,color);

Логично предположить, что условие может быть не выполнено, и тогда буфер останется неинициализированным. В этом случае его нельзя использовать для добавления к строке ab.

Чтобы исправить ситуацию, стоит изменить код следующим образом:

if (color != -1 || bold != 0){  snprintf(seq,64,"\033[%d;%d;49m",bold,color);  abAppend(ab,seq,strlen(seq));}

Ошибка N70; Странная маска

Предупреждение PVS-Studio: V547 Expression is always false. tasks.c 896

#ifndef portPRIVILEGE_BIT  #define portPRIVILEGE_BIT ( ( UBaseType_t ) 0x00 )#endifstatic void prvInitialiseNewTask(...., UBaseType_t uxPriority, ....){  StackType_t *pxTopOfStack;  UBaseType_t x;  #if (portNUM_PROCESSORS < 2)  xCoreID = 0;  #endif  #if( portUSING_MPU_WRAPPERS == 1 )    /* Should the task be created in privileged mode? */    BaseType_t xRunPrivileged;    if( ( uxPriority & portPRIVILEGE_BIT ) != 0U )    {      xRunPrivileged = pdTRUE;    }    else    {      xRunPrivileged = pdFALSE;    }  ....}

Константа portPRIVILEGE_BIT имеет значение 0. Поэтому странно использовать его как маску:

if( ( uxPriority & portPRIVILEGE_BIT ) != 0U )

Ошибка N71, Утечка памяти

Предупреждение PVS-Studio: V773 The function was exited without releasing the 'sm' pointer. A memory leak is possible. esp_wpa2.c 753

static int eap_peer_sm_init(void){  int ret = 0;  struct eap_sm *sm;  ....  sm = (struct eap_sm *)os_zalloc(sizeof(*sm));  if (sm == NULL) {    return ESP_ERR_NO_MEM;  }  s_wpa2_data_lock = xSemaphoreCreateRecursiveMutex();  if (!s_wpa2_data_lock) {    wpa_printf(MSG_ERROR, ".......");  // NOLINT(clang-analyzer-unix.Malloc)    return ESP_ERR_NO_MEM;             // <=  }  ....}

Если функция xSemaphoreCreateRecursiveMutex не сможет создать мьютекс, то функция eap_peer_sm_init завершит свою работу и при этом произойдёт утечка памяти. Как я понимаю, следует добавить вызов функции os_free для очистки памяти:

  s_wpa2_data_lock = xSemaphoreCreateRecursiveMutex();  if (!s_wpa2_data_lock) {    wpa_printf(MSG_ERROR, ".......");    os_free(sm);    return ESP_ERR_NO_MEM;  }

Что интересно, компилятор Clang тоже предупреждает об этой ошибке. Однако автор кода почему-то проигнорировал и даже специально подавил соответствующее предупреждение:

// NOLINT(clang-analyzer-unix.Malloc)

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

Заключение

Спасибо за внимание. Как видите, ошибок весьма много. А это ведь был только беглый просмотр неполного отчёта. Надеюсь, Юрий Попов примет эстафету и опишет ещё больше ошибок в своей последующей статье :).

Используйте статический анализатор PVS-Studio регулярно. Это позволит:

1. Находить многие ошибки на раннем этапе, что существенно сократит расходы на их обнаружение и исправление;
2. Находя и исправляя глупые опечатки и прочие ляпы с помощью статического анализа, вы высвободите время, которое можно потратить на более высокоуровневый обзор кода и алгоритмов;
3. Лучше контролировать качество кода новичков и быстрее обучать их писать красивый надежный код;
4. Если речь идёт о программном обеспечении для встраиваемых устройств, то очень важно устранить как можно больше ошибок до выпуска устройств в эксплуатацию. Поэтому любая дополнительно найденная ошибка с помощью анализатора кода, это здорово. Каждая незамеченная ошибка в программно-аппаратном устройстве потенциально несёт репутационные риски и затраты на обновление прошивок.

Приглашаю скачать и попробовать демонстрационную версию анализатора PVS-Studio. Также напоминаю, что если вы разрабатываете открытый проект или используете анализатор в академических целях, то для таких случаев мы предлагаем несколько вариантов бесплатных лицензий. Не ждите, когда коварный баг съест вашу ногу, начните использовать PVS-Studio прямо сейчас.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Espressif IoT Development Framework: 71 Shots in the Foot.

Относительно недавно состоялся релиз фреймворка Qt 6, и это стало поводом вновь проверить его с помощью PVS-Studio. В статье будут рассмотрены различные интересные ошибки, например, связанные с обработкой дат. Обнаружение всех этих ошибок хорошо демонстрирует пользу, которую может получить проект от использования таких инструментов, как PVS-Studio, особенно если они применяются регулярно.

Это классическая статья о проверке открытого проекта, которая пополнит нашу "доказательную базу" полезности и эффективности использования PVS-Studio для контроля качества кода. Хотя мы уже писали про поверку проекта Qt (в 2011, в 2014 и в 2018), очень полезно сделать это вновь. Так, мы на практике демонстрируем простую, но очень важную мысль: статический анализ должен применяться регулярно!

Наши статьи показывают, что анализатор PVS-Studio умеет находить множество разнообразнейших ошибок. И, как правило, авторы проектов быстро исправляют описанные нами ошибки. Однако всё это не имеет ничего общего с правильной и полезной методикой регулярного применения статических анализаторов. Когда анализатор встроен в процесс разработки, это позволяет быстро находить ошибки в новом или изменённом коде и тем самым их исправление обходится максимально дёшево.

Всё, теория заканчивается. Давайте посмотрим, что интересного нас ждёт в коде. А пока вы будете читать статью, предлагаю скачать PVS-Studio и запросить демонстрационный ключ, чтобы посмотреть, что интересного найдётся в ваших собственных проектах :).

Даты

Кажется, намечается ещё один паттерн кода, где любят собираться ошибки. Он, конечно, не такой масштабный, как функции сравнения или последние строки в однотипных блоках. Речь идёт про фрагменты кода, работающие с датами. Наверное, такой код сложно тестировать, и в итоге эти недотестированные функции будут давать некорректный результат на определённых наборах входных данных. Про пару таких случаев уже было рассказано в статье "Почему PVS-Studio не предлагает автоматические правки кода".

Встретились ошибки обработки дат и в Qt. Давайте с них и начнём.

Фрагмент N1: неправильная интерпретация статуса ошибки

Для начала нам следует посмотреть, как устроена функция, возвращающая номер месяца по его сокращённому названию.

static const char qt_shortMonthNames[][4] = {    "Jan", "Feb", "Mar", "Apr", "May", "Jun",    "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};static int fromShortMonthName(QStringView monthName){  for (unsigned int i = 0;       i < sizeof(qt_shortMonthNames) / sizeof(qt_shortMonthNames[0]); ++i)  {    if (monthName == QLatin1String(qt_shortMonthNames[i], 3))      return i + 1;  }  return -1;}

В случае успеха функция возвращает номер месяца (значение от 1 до 12). Если имя месяца некорректно, то функция возвращает отрицательное значение (-1). Обратите внимание, что функция не может вернуть значение 0.

Но там, где только что рассмотренная функция используется, программист как раз рассчитывает, что в качестве статуса ошибки ему будет возвращено нулевое значение. Фрагмент кода, некорректно использующий функцию fromShortMonthName:

QDateTime QDateTime::fromString(QStringView string, Qt::DateFormat format){  ....  month = fromShortMonthName(parts.at(1));  if (month)    day = parts.at(2).toInt(&ok);  // If failed, try day then month  if (!ok || !month || !day) {    month = fromShortMonthName(parts.at(2));    if (month) {      QStringView dayPart = parts.at(1);      if (dayPart.endsWith(u'.'))        day = dayPart.chopped(1).toInt(&ok);    }  }  ....}

Проверки номера месяца на равенство нулю никогда не сработают, и программа продолжит выполняться с некорректным отрицательным номером месяца. Анализатор PVS-Studio видит здесь целый букет несостыковок, о чем сообщает сразу четырьмя сообщениями:

• V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4907
• V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4911
• V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4913
• V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4921

Фрагмент N2: ошибка логики обработки даты

Для начала посмотрим на реализацию функции, возвращающей количество секунд.

enum {  ....  MSECS_PER_DAY = 86400000,  ....  SECS_PER_MIN = 60,};int QTime::second() const{    if (!isValid())        return -1;    return (ds() / 1000)%SECS_PER_MIN;}

Рассмотренная функция может вернуть значение в диапазоне [0..59] или статус ошибки -1.

В одном месте эта функция используется очень странным образом:

static qint64 qt_mktime(QDate *date, QTime *time, ....){  ....  } else if (yy == 1969 && mm == 12 && dd == 31             && time->second() == MSECS_PER_DAY - 1) {      // There was, of course, a last second in 1969, at time_t(-1); we won't      // rescue it if it's not in normalised form, and we don't know its DST      // status (unless we did already), but let's not wantonly declare it      // invalid.  } else {  ....}

Предупреждение PVS-Studio: V560 [CWE-570] A part of conditional expression is always false: time->second() == MSECS_PER_DAY 1. qdatetime.cpp 2488

Согласно комментарию, если что-то пошло не так, то лучше ничего не делать. Однако, условие всегда будет ложным, поэтому всегда выполняется else-ветка.

Ошибочно вот это сравнение:

time->second() == MSECS_PER_DAY - 1

MSECS_PER_DAY 1 это 86399999. Функция second, как мы уже знаем, никак не может вернуть такое значение. Таким образом, здесь какая-то логическая ошибка и код заслуживает пристального внимания разработчиков.

Сильная сторона статических анализаторов кода в том, что они проверяют все ситуации, независимо от частоты их выполнения. Таким образом, статический анализ хорошо дополняет юнит-тесты и другие методы контроля качества кода.

Опечатки

Фрагмент N3: неожиданно, мы поговорим о HTML!

QString QPixelTool::aboutText() const{  const QList<QScreen *> screens = QGuiApplication::screens();  const QScreen *windowScreen = windowHandle()->screen();  QString result;  QTextStream str(&result);  str << "<html></head><body><h2>Qt Pixeltool</h2><p>Qt " << QT_VERSION_STR    << "</p><p>Copyright (C) 2017 The Qt Company Ltd.</p><h3>Screens</h3><ul>";  for (const QScreen *screen : screens)    str << "<li>" << (screen == windowScreen ? "* " : "  ")        << screen << "</li>";  str << "<ul></body></html>";  return result;}

Предупреждение PVS-Studio: V735 Possibly an incorrect HTML. The "</ body>" closing tag was encountered, while the "</ ul>" tag was expected. qpixeltool.cpp 707

В PVS-Studio есть диагностики, которые не только проверят сам код, но и выискивают аномалии в строковых константах. Здесь как раз сработала одна из таких диагностик. Это достаточно редкий случай, зато этим он и примечательный.

Дважды используется тег открытия списка. Это явная опечатка. Первый тег, должен открывать список, а второй закрывать. Правильный код:

str << "</ul></body></html>";

Фрагмент N4: повторная проверка в условии

class Node{  ....  bool isGroup() const { return m_nodeType == Group; }  ....};void DocBookGenerator::generateDocBookSynopsis(const Node *node){  ....  if (node->isGroup() || node->isGroup()      || node->isSharedCommentNode() || node->isModule()      || node->isJsModule() || node->isQmlModule() || node->isPageNode())    return;  ....}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions to the left and to the right of the '||' operator: node->isGroup() || node->isGroup() docbookgenerator.cpp 2599

Простая опечатка, но её исправление зависит от того, чего на самом деле хотели достичь в этом коде. Если проверка просто дублируется, то её стоит просто удалить. Но возможен и другой сценарий: не проверено какое-то другое нужное условие.

Фрагмент N5: создание лишней локальной переменной

void MainWindow::addToPhraseBook(){  ....  QString selectedPhraseBook;  if (phraseBookList.size() == 1) {    selectedPhraseBook = phraseBookList.at(0);    if (QMessageBox::information(this, tr("Add to phrase book"),          tr("Adding entry to phrasebook %1").arg(selectedPhraseBook),           QMessageBox::Ok | QMessageBox::Cancel, QMessageBox::Ok)                          != QMessageBox::Ok)      return;  } else {    bool okPressed = false;    QString selectedPhraseBook =       QInputDialog::getItem(this, tr("Add to phrase book"),                            tr("Select phrase book to add to"),                            phraseBookList, 0, false, &okPressed);    if (!okPressed)      return;  }  MessageItem *currentMessage = m_dataModel->messageItem(m_currentIndex);  Phrase *phrase = new Phrase(currentMessage->text(),                              currentMessage->translation(),                              QString(), nullptr);  phraseBookHash.value(selectedPhraseBook)->append(phrase);}

Кстати, если хотите, можете проверить свою внимательность и самостоятельно поискать ошибку. А чтобы вы сразу случайно не увидели ответ, добавлю сюда единорожку из старой коллекции. Возможно, вы даже его ещё не видели :).

Предупреждение PVS-Studio: V561 [CWE-563] It's probably better to assign value to 'selectedPhraseBook' variable than to declare it anew. Previous declaration: mainwindow.cpp, line 1303. mainwindow.cpp 1313

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

QString selectedPhraseBook =

В результате в else-блоке появилась ещё одна локальная строковая переменная, которая инициализируется, но не используется. Тем временем переменная с таким же именем во внешнем блоке останется пустой.

Фрагмент N6: приоритет операций

Классический паттерн ошибки, который встречается весьма часто.

bool QQmlImportInstance::resolveType(....){  ....  if (int icID = containingType.lookupInlineComponentIdByName(typeStr) != -1)  {    *type_return = containingType.lookupInlineComponentById(icID);  } else {    auto icType = createICType();    ....  }  ....}

Предупреждение PVS-Studio: V593 [CWE-783] Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. qqmlimport.cpp 754

Значение переменной icID всегда будет иметь значение 0 или 1. Это явно не то, что задумывалось. Причина: в начале происходит сравнение с -1, а только затем инициализация переменной icID.

Современный синтаксис C++ позволяет корректно записать это условие следующим образом:

if (int icID = containingType.lookupInlineComponentIdByName(typeStr);    icID != -1)

Кстати, очень похожую ошибку мы уже обнаруживали в Qt:

char ch;while (i < dataLen && ((ch = data.at(i) != '\n') && ch != '\r'))  ++i;

Но пока на вооружение не будет взят такой анализатор кода, как PVS-Studio, программисты вновь и вновь будут допускать такие ошибки. Никто не совершенен. И да, это тонкий намёк внедрить PVS-Studio :).

Фрагмент N7: коварное деление по модулю

Часто бывает необходимо определить, делится число без остатка на 2 или нет. Правильный вариант это поделить по модулю два и проверить результат:

if (A % 2 == 1)

Но программисты вновь и вновь ошибаются и пишут что-то типа этого:

if (A % 1 == 1)

Это естественно неправильно, так как остаток от деления по модулю на один это всегда ноль. Не обошлось без этой ошибки и в Qt:

bool loadQM(Translator &translator, QIODevice &dev, ConversionData &cd){  ....  case Tag_Translation: {    int len = read32(m);    if (len % 1) {                                             // <=      cd.appendError(QLatin1String("QM-Format error"));      return false;    }    m += 4;    QString str = QString((const QChar *)m, len/2);  ....}

Предупреждение PVS-Studio: V1063 The modulo by 1 operation is meaningless. The result will always be zero. qm.cpp 549

Фрагмент N8: перезаписывание значения

QString Node::qualifyQmlName(){  QString qualifiedName = m_name;  if (m_name.startsWith(QLatin1String("QML:")))    qualifiedName = m_name.mid(4);  qualifiedName = logicalModuleName() + "::" + m_name;  return qualifiedName;}

Предупреждение PVS-Studio: V519 [CWE-563] The 'qualifiedName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1227, 1228. node.cpp 1228

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

QString qualifiedName = m_name;if (m_name.startsWith(QLatin1String("QML:")))  qualifiedName = m_name.mid(4);qualifiedName = logicalModuleName() + "::" + qualifiedName;return qualifiedName;

Фрагмент N9: copy-paste

class Q_CORE_EXPORT QJsonObject{  ....  bool operator<(const iterator& other) const  { Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }  bool operator<=(const iterator& other) const  { Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }  ....}

Прудпреждение PVS-Studio: V524 It is odd that the body of '<=' function is fully equivalent to the body of '<' function. qjsonobject.h 155

Такие скучные функции, как операторы сравнения, никто не проверяет. На них обычно не пишут тесты. Их не просматривают или делают это очень быстро на code review. А зря. Статический анализ кода здесь как нельзя кстати. Анализатор не устаёт и с удовольствием проверяет даже такой скучный код.

Реализация операторов < и <= совпадают. Это явно неправильно. Скорее всего, код писался методом Copy-Paste, и затем забыли изменить всё что нужно в скопированном коде. Правильно:

bool operator<(const iterator& other) const{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }bool operator<=(const iterator& other) const{ Q_ASSERT(item.o == other.item.o); return item.index <= other.item.index; }

Фрагмент N10: static_cast / dynamic_cast

void QSGSoftwareRenderThread::syncAndRender(){  ....  bool canRender = wd->renderer != nullptr;  if (canRender) {     auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);     if (softwareRenderer)       softwareRenderer->setBackingStore(backingStore);  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'softwareRenderer' is always true. qsgsoftwarethreadedrenderloop.cpp 510

В начале рассмотрим вот эту проверку:

bool canRender = wd->renderer != nullptr;if (canRender) {

Благодаря ей можно быть уверенным, что внутри тела условного оператора значение указателя wd->renderer всегда точно ненулевое. Тогда непонятно, что же хотят проверить следующим кодом?

auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);if (softwareRenderer)

Если указатель wd->renderer ненулевой, то и указатель softwareRenderer точно ненулевой. Есть подозрение, что здесь опечатка, которая состоит в том, что на самом деле следовало использовать dynamic_cast. В этом случае код начинает приобретать смысл. Если преобразование типа невозможно, оператор dynamic_cast возвращает nullptr, и это возвращенное значение, естественно, следует проверять. Впрочем, возможно, я неправильно интерпретировал ситуацию и код нужно исправлять другим способом.

Фрагмент N11: скопировали блок кода и забыли изменить

void *QQuickPath::qt_metacast(const char *_clname){  if (!_clname) return nullptr;  if (!strcmp(_clname, qt_meta_stringdata_QQuickPath.stringdata0))    return static_cast<void*>(this);  if (!strcmp(_clname, "QQmlParserStatus"))    return static_cast< QQmlParserStatus*>(this);  if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))   // <=    return static_cast< QQmlParserStatus*>(this);  if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))   // <=    return static_cast< QQmlParserStatus*>(this);  return QObject::qt_metacast(_clname);}

Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 2719, 2721. moc_qquickpath_p.cpp 2721

Эти две строчки:

if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))  return static_cast< QQmlParserStatus*>(this);

Были размножены с помощью Copy-Paste. После чего они не были модифицированы и не имеют смысла.

Фрагмент N12: переполнение из-за не там поставленной скобки

int m_offsetFromUtc;....void QDateTime::setMSecsSinceEpoch(qint64 msecs){  ....  if (!add_overflow(msecs, qint64(d->m_offsetFromUtc * 1000), &msecs))    status |= QDateTimePrivate::ValidWhenMask;  ....}

Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands of the 'd->m_offsetFromUtc * 1000' operator to the 'qint64' type, not the result. qdatetime.cpp 3922

Программист предвидит ситуацию, что при умножении переменной типа int на 1000 может произойти переполнение. Чтобы этого избежать, он планирует использовать при умножении 64-битный тип qint64. И использует явное приведение типа.

Вот только толку от этого приведения типа никакого нет. В начале всё равно произойдёт переполнение. И только затем выполнится приведение типа. Правильный вариант:

add_overflow(msecs, qint64(d->m_offsetFromUtc) * 1000, &msecs)

Фрагмент N13: не полностью инициализированный массив

class QPathEdge{  ....private:  int m_next[2][2];  ....};inline QPathEdge::QPathEdge(int a, int b)    : flag(0)    , windingA(0)    , windingB(0)    , first(a)    , second(b)    , angle(0)    , invAngle(0){    m_next[0][0] = -1;    m_next[1][0] = -1;    m_next[0][0] = -1;    m_next[1][0] = -1;}

Предупреждения PVS-Studio:

• V1048 [CWE-1164] The 'm_next[0][0]' variable was assigned the same value. qpathclipper_p.h 301
• V1048 [CWE-1164] The 'm_next[1][0]' variable was assigned the same value. qpathclipper_p.h 302

Перед нами неудачная попытка инициализировать массив размером 2x2. Два элемента инициализируются повторно, и два остаются неинициализированными. Правильный вариант:

m_next[0][0] = -1;m_next[0][1] = -1;m_next[1][0] = -1;m_next[1][1] = -1;

Я очень люблю такие примеры ошибок, которые встречаются в коде профессиональных разработчиков. Это как раз такой случай. Он показывает, что любой может опечататься, и поэтому статический анализ является вашим другом. Дело в том, что я уже десяток лет веду бой со скептиками, которые уверены, что такие ошибки можно встретить только в лабораторных работах студентов и что они такие ошибки никогда не делают :). Ещё 10 лет назад я написал заметку "Миф второй профессиональные разработчики не допускают глупых ошибок", и с тех пор, естественно, ничего не изменилось. Люди всё также делают такие ошибки и всё также утверждают, что это не так :).

Ошибки в логике

Фрагмент N14: недостижимый код

void QmlProfilerApplication::tryToConnect(){  Q_ASSERT(!m_connection->isConnected());  ++ m_connectionAttempts;  if (!m_verbose && !(m_connectionAttempts % 5)) {// print every 5 seconds    if (m_verbose) {      if (m_socketFile.isEmpty())        logError(          QString::fromLatin1("Could not connect to %1:%2 for %3 seconds ...")          .arg(m_hostName).arg(m_port).arg(m_connectionAttempts));      else        logError(          QString::fromLatin1("No connection received on %1 for %2 seconds ...")          .arg(m_socketFile).arg(m_connectionAttempts));    }  }  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'm_verbose' is always false. qmlprofilerapplication.cpp 495

Этот код никогда ничего не запишет в лог. Причиной являются противоположные условия:

if (!m_verbose && ....) {  if (m_verbose) {

Фрагмент N15: перетирание значения переменной

void QRollEffect::scroll(){  ....  if (currentHeight != totalHeight) {      currentHeight = totalHeight * (elapsed/duration)          + (2 * totalHeight * (elapsed%duration) + duration)          / (2 * duration);      // equiv. to int((totalHeight*elapsed) / duration + 0.5)      done = (currentHeight >= totalHeight);  }  done = (currentHeight >= totalHeight) &&         (currentWidth >= totalWidth);  ....}

Предупреждение PVS-Studio: V519 [CWE-563] The 'done' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 509, 511. qeffects.cpp 511

Весь условный оператор не имеет смысла, так как значение переменной done всё равно тут же перезаписывается. Возможно, здесь не хватает ключевого слова else.

Фрагмент N16-N20: перетирание значения переменной

Альтернативный вариант перетирания значения переменной.

bool QXmlStreamWriterPrivate::finishStartElement(bool contents){  ....  if (inEmptyElement) {    ....    lastNamespaceDeclaration = tag.namespaceDeclarationsSize;   // <=    lastWasStartElement = false;  } else {    write(">");  }  inStartElement = inEmptyElement = false;  lastNamespaceDeclaration = namespaceDeclarations.size();      // <=  return hadSomethingWritten;}

Предупреждение PVS-Studio: V519 [CWE-563] The 'lastNamespaceDeclaration' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3030, 3036. qxmlstream.cpp 3036

Возможно, первая запись значения в переменную lastNamespaceDeclaration является лишней и её можно удалить. А возможно, перед нами серьезная логическая ошибка.

Есть ещё 4 предупреждения, указывающих на такой же паттерн ошибки:

• V519 [CWE-563] The 'last' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 609, 637. qtextengine.cpp 637
• V519 [CWE-563] The 'm_dirty' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1014, 1017. qquickshadereffect.cpp 1017
• V519 [CWE-563] The 'changed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 122, 128. qsgdefaultspritenode.cpp 128
• V519 [CWE-563] The 'eaten' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 299, 301. qdesigner.cpp 301

Фрагмент N21: путаница между нулевым указателем и пустой строкой

// this could become a list of all languages used for each writing// system, instead of using the single most common language.static const char languageForWritingSystem[][6] = {    "",     // Any    "en",  // Latin    "el",  // Greek    "ru",  // Cyrillic    ...... // Нулевых указателей нет. Используются пустые строковые литералы.    "", // Symbol    "sga", // Ogham    "non", // Runic    "man" // N'Ko};static void populateFromPattern(....){  ....  for (int j = 1; j < QFontDatabase::WritingSystemsCount; ++j) {    const FcChar8 *lang = (const FcChar8*) languageForWritingSystem[j];    if (lang) {  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'lang' is always true. qfontconfigdatabase.cpp 462

В массиве languageForWritingSystem нет нулевых указателей. Поэтому проверка if(lang) не имеет смысла. Зато в массиве есть пустые строки. Быть может, хотелось сделать проверку именно на пустую строку? Если да, тогда корректный код должен выглядеть так:

if (strlen(lang) != 0) {

Или можно ещё проще написать:

if (lang[0] != '\0') {

Фрагмент N22: странная проверка

bool QNativeSocketEnginePrivate::createNewSocket(....){  ....  int socket = qt_safe_socket(domain, type, protocol, O_NONBLOCK);  ....  if (socket < 0) {    ....    return false;  }  socketDescriptor = socket;  if (socket != -1) {    this->socketProtocol = socketProtocol;    this->socketType = socketType;  }  return true;}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'socket != 1' is always true. qnativesocketengine_unix.cpp 315

Условие socket != -1 всегда истинно, так как выше происходит выход из функции, если значение переменной socket отрицательное.

Фрагмент N23: так что же всё-таки должна вернуть функция?

bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent){  Q_D(QSqlTableModel);  if (parent.isValid() || row < 0 || count <= 0)    return false;  else if (row + count > rowCount())    return false;  else if (!count)    return true;  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression '!count' is always false. qsqltablemodel.cpp 1110

Для упрощения выделю самое главное:

if (.... || count <= 0)  return false;....else if (!count)  return true;

Первая проверка говорит нам, что если значение count меньше или равно 0, то это ошибочное состояние и функция должна вернуть false. Однако ниже мы видим точное сравнение этой переменной с нулём, и этот случай уже интерпретируется по-другому: функция должна вернуть true.

Здесь явно что-то не так. Я подозреваю, что на самом деле проверка должна быть не <=, а просто <. Тогда код обретает смысл:

bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent){  Q_D(QSqlTableModel);  if (parent.isValid() || row < 0 || count < 0)    return false;  else if (row + count > rowCount())    return false;  else if (!count)    return true;  ....}

Фрагмент N24: лишний статус?

В следующем коде переменная identifierWithEscapeChars выглядит просто как лишняя сущность. Или это логическая ошибка? Или код не дописан? К моменту второй проверки эта переменная в любом случае всегда будет равна true.

int Lexer::scanToken(){  ....  bool identifierWithEscapeChars = false;  ....  if (!identifierWithEscapeChars) {    identifierWithEscapeChars = true;    ....  }  ....  if (identifierWithEscapeChars) {    // <=    ....  }  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'identifierWithEscapeChars' is always true. qqmljslexer.cpp 817

Фрагмент N25: что делать с девятью объектами?

bool QFont::fromString(const QString &descrip){  ....  const int count = l.count();  if (!count || (count > 2 && count < 9) || count == 9 || count > 17 ||      l.first().isEmpty()) {    qWarning("QFont::fromString: Invalid description '%s'",             descrip.isEmpty() ? "(empty)" : descrip.toLatin1().data());    return false;  }  setFamily(l[0].toString());  if (count > 1 && l[1].toDouble() > 0.0)    setPointSizeF(l[1].toDouble());  if (count == 9) {                           // <=    setStyleHint((StyleHint) l[2].toInt());    setWeight(QFont::Weight(l[3].toInt()));    setItalic(l[4].toInt());    setUnderline(l[5].toInt());    setStrikeOut(l[6].toInt());    setFixedPitch(l[7].toInt());  } else if (count >= 10) {  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'count == 9' is always false. qfont.cpp 2142

Как должна себя вести функция, если переменная count равна 9? С одной стороны, функция должна выдать предупреждение и завершить свою работу. Ведь явно написано:

if (.... || count == 9 || ....) {  qWarning(....);  return false;}

С другой стороны, для 9 объектов предусмотрено выполнение специального кода:

if (count == 9) {  setStyleHint((StyleHint) l[2].toInt());  setWeight(QFont::Weight(l[3].toInt()));  setItalic(l[4].toInt());  ....}

Этот код, конечно, никогда не выполняется. Код ждёт, чтобы его пришли и исправили :).

Нулевые указатели

Фрагмент N26-N42: использование указателя до его проверки

class __attribute__((visibility("default"))) QMetaType {  ....  const QtPrivate::QMetaTypeInterface *d_ptr = nullptr;};QPartialOrdering QMetaType::compare(const void *lhs, const void *rhs) const{    if (!lhs || !rhs)        return QPartialOrdering::Unordered;    if (d_ptr->flags & QMetaType::IsPointer)        return threeWayCompare(*reinterpret_cast<const void * const *>(lhs),                               *reinterpret_cast<const void * const *>(rhs));    if (d_ptr && d_ptr->lessThan) {        if (d_ptr->equals && d_ptr->equals(d_ptr, lhs, rhs))            return QPartialOrdering::Equivalent;        if (d_ptr->lessThan(d_ptr, lhs, rhs))            return QPartialOrdering::Less;        if (d_ptr->lessThan(d_ptr, rhs, lhs))            return QPartialOrdering::Greater;        if (!d_ptr->equals)            return QPartialOrdering::Equivalent;    }    return QPartialOrdering::Unordered;}

Предупреждение PVS-Studio: V595 [CWE-476] The 'd_ptr' pointer was utilized before it was verified against nullptr. Check lines: 710, 713. qmetatype.cpp 710

Ошибка на первый взгляд может быть не заметна. Но на самом деле всё просто. Проследим, как работают с указателем d_ptr:

if (d_ptr->flags & ....)if (d_ptr && ....)

Вначале выполняется условие, в котором указатель разыменовывается. И только из следующей проверки становится ясно, что этот указатель, оказывается, может быть нулевой.

Это один из самых распространённых паттернов ошибки в языке C и С++. Пруфы. В исходных Qt кодах тоже встречается немало ошибок этой разновидности:

• V595 [CWE-476] The 'self' pointer was utilized before it was verified against nullptr. Check lines: 1346, 1351. qcoreapplication.cpp 1346
• V595 [CWE-476] The 'currentTimerInfo' pointer was utilized before it was verified against nullptr. Check lines: 636, 641. qtimerinfo_unix.cpp 636
• V595 [CWE-476] The 'lib' pointer was utilized before it was verified against nullptr. Check lines: 325, 333. qlibrary.cpp 325
• V595 [CWE-476] The 'fragment.d' pointer was utilized before it was verified against nullptr. Check lines: 2262, 2266. qtextcursor.cpp 2262
• V595 [CWE-476] The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1581, 1583. qapplication.cpp 1581
• V595 [CWE-476] The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1593, 1595. qapplication.cpp 1593
• V595 [CWE-476] The 'newHandle' pointer was utilized before it was verified against nullptr. Check lines: 873, 879. qsplitter.cpp 873
• V595 [CWE-476] The 'targetModel' pointer was utilized before it was verified against nullptr. Check lines: 454, 455. qqmllistmodel.cpp 454
• V595 [CWE-476] The 'childIface' pointer was utilized before it was verified against nullptr. Check lines: 102, 104. qaccessiblequickitem.cpp 102
• V595 [CWE-476] The 'e' pointer was utilized before it was verified against nullptr. Check lines: 94, 98. qquickwindowmodule.cpp 94
• V595 [CWE-476] The 'm_texture' pointer was utilized before it was verified against nullptr. Check lines: 235, 239. qsgplaintexture.cpp 235
• V595 [CWE-476] The 'm_unreferencedPixmaps' pointer was utilized before it was verified against nullptr. Check lines: 1140, 1148. qquickpixmapcache.cpp 1140
• V595 [CWE-476] The 'camera' pointer was utilized before it was verified against nullptr. Check lines: 263, 264. assimpimporter.cpp 263
• V595 [CWE-476] The 'light' pointer was utilized before it was verified against nullptr. Check lines: 273, 274. assimpimporter.cpp 273
• V595 [CWE-476] The 'channel' pointer was utilized before it was verified against nullptr. Check lines: 337, 338. assimpimporter.cpp 337
• V595 [CWE-476] The 'm_fwb' pointer was utilized before it was verified against nullptr. Check lines: 2492, 2500. designerpropertymanager.cpp 2492

Фрагмент N43: использование указателя до его проверки в рамках одного выражения

Та же самая ситуация, что и выше. Однако в этот раз разыменование и проверка указателя находятся в одном выражении. Классическая ошибка, возникающая из-за невнимательности при написании кода и при последующем code-review.

void QFormLayoutPrivate::updateSizes(){  ....  QFormLayoutItem *field = m_matrix(i, 1);  ....  if (userHSpacing < 0 && !wrapAllRows && (label || !field->fullRow) && field)  ....}

Предупреждение PVS-Studio: V713 [CWE-476] The pointer 'field' was utilized in the logical expression before it was verified against nullptr in the same logical expression. qformlayout.cpp 405

Минутка отдыха

Я устал. Думаю, устали и читатели. Тут устанешь, даже если просто бегло просматривать текст статьи :). Поэтому я пошёл за второй чашечкой кофе. Первую я выпил где-то на рубеже 12-ого фрагмента. Приглашаю и читателей сходить за любимым напитком.

И пока все ушли на кухню, рекламная пауза. Приглашаю команду, занимающуюся разработкой проекта Qt рассмотреть вопрос приобретения лицензии на анализатор кода PVS-Studio. Запросить прайс можно здесь. С нашей стороны поддержка и помощь в настройке. Да, согласен, я сегодня более навязчив, чем обычно. Это эксперимент :).

Фрагмент N44-N72: нет проверки, что вернула функция malloc

void assignData(const QQmlProfilerEvent &other){  if (m_dataType & External) {    uint length = m_dataLength * (other.m_dataType / 8);    m_data.external = malloc(length);                          // <=    memcpy(m_data.external, other.m_data.external, length);    // <=  } else {    memcpy(&m_data, &other.m_data, sizeof(m_data));  }}

Предупреждение PVS-Studio: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 277, 276. qqmlprofilerevent_p.h 277

Нельзя просто взять и использовать указатель, который вернула функция malloc. Нужно обязательно проверить, не является ли этот указатель нулевым, даже если очень лень всем этим заниматься. Для этого есть 4 причины, которые описаны в статье "Почему важно проверять, что вернула функция malloc".

Перед нами один из случаев, где отсутствует необходимая проверка. Есть и другие предупреждения, но из-за их количества включать весь список в статью не хочется. На всякий случай, я выписал 28 предупреждений в файл: qt6-malloc.txt. Но на самом деле разработчикам, конечно, лучше самим перепроверить проект и самостоятельно изучить предупреждения. У меня не было задачи выявить как можно больше ошибок.

Что интересно, на фоне важных забытых проверок есть совершенно ненужные. Речь идёт о вызове оператора new, который в случае ошибки выделения памяти сгенерирует исключение std::bad_alloc. Вот один из примеров такой избыточной проверки:

static QImageScaleInfo* QImageScale::qimageCalcScaleInfo(....){  ....  QImageScaleInfo *isi;  ....  isi = new QImageScaleInfo;  if (!isi)    return nullptr;  ....}

Предупреждение PVS-Studio: V668 [CWE-570] There is no sense in testing the 'isi' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qimagescale.cpp 245

P.S. Здесь читатели всегда задают вопрос, учитывает ли анализатор placement new или "new (std::nothrow) T"? Да, учитывает и не выдаёт для них ложные срабатывания.

Избыточный код ("код с запахом")

Встречаются ситуации, когда анализатор выдаёт предупреждения на код, который является корректным, но избыточным. Например, есть повторная проверка одной и той же переменной. При этом даже затруднительно сказать, ложное это срабатывание или нет. Формально анализатор прав, но и настоящую ошибку он не нашёл.

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

Часто я вообще не рассматриваю в статьях такие предупреждения. Это неинтересно. Однако в проекте Qt вдруг таких случаев оказалось удивительно много. Явно больше, чем обычно. Поэтому я решил уделить этому немного внимания и разобрать несколько таких случаев. Думаю, будет полезно провести рефакторинг этих и многих других аналогичных мест. Для этого потребуется использовать полный отчёт, не руководствоваться тем, что я выпишу в статью.

Итак, взглянем на несколько показательных случаев.

Фрагмент N73: "код с запахом" обратная проверка

void QQuick3DSceneManager::setWindow(QQuickWindow *window){  if (window == m_window)    return;  if (window != m_window) {    if (m_window)      disconnect(....);    m_window = window;    connect(....);    emit windowChanged();  }}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'window != m_window' is always true. qquick3dscenemanager.cpp 60

Если window==m_window, то функция завершает работу. Последующая обратная проверка не имеет никакого смысла и только загромождает код.

Фрагмент N74: "код с запахом" странная инициализация

QModelIndex QTreeView::moveCursor(....){  ....  int vi = -1;  if (vi < 0)    vi = qMax(0, d->viewIndex(current));  ....}

Предупреждение PVS-Stduio: V547 [CWE-571] Expression 'vi < 0' is always true. qtreeview.cpp 2219

Что это? Зачем так писать? Код можно упростить до одной строки:

int vi = qMax(0, d->viewIndex(current));

Фрагмент N75: "код с запахом" недостижимый код

bool copyQtFiles(Options *options){  ....  if (unmetDependencies.isEmpty()) {    if (options->verbose) {      fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",              qPrintable(sourceFileName));    }  } else {    if (unmetDependencies.isEmpty()) {      if (options->verbose) {        fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",                  qPrintable(sourceFileName));      }    } else {      fprintf(stdout, "  -- Skipping %s. It has unmet dependencies: %s.\n",              qPrintable(sourceFileName),              qPrintable(unmetDependencies.join(QLatin1Char(','))));    }  }  ....}

Предупреждение PVS-Studio: V571 [CWE-571] Recurring check. The 'if (unmetDependencies.isEmpty())' condition was already verified in line 2203. main.cpp 2209

На первый взгляд перед нами респектабельный код, формирующий подсказку. Но давайте приглядимся. Если первый раз условие unmetDependencies.isEmpty() выполнилось, то второй раз этого уже не произойдёт. Это нестрашно, так как автор планировал вывести то же самое сообщение. Настоящей ошибки нет, но код переусложнён. Он может быть упрощен до следующего варианта:

bool copyQtFiles(Options *options){  ....  if (unmetDependencies.isEmpty()) {    if (options->verbose) {      fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",              qPrintable(sourceFileName));    }  } else {    fprintf(stdout, "  -- Skipping %s. It has unmet dependencies: %s.\n",            qPrintable(sourceFileName),            qPrintable(unmetDependencies.join(QLatin1Char(','))));  }  ....}

Фрагмент N76: "код с запахом" сложный тернарный оператор

bool QDockAreaLayoutInfo::insertGap(....){  ....  QDockAreaLayoutItem new_item    = widgetItem == nullptr      ? QDockAreaLayoutItem(subinfo)      : widgetItem ? QDockAreaLayoutItem(widgetItem)                    : QDockAreaLayoutItem(placeHolderItem);  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'widgetItem' is always true. qdockarealayout.cpp 1167

Возможно, здесь мы имеем дело с настоящей ошибкой. Но я больше склоняюсь к тому, что этот код несколько раз менялся и в результате получился бессмысленно сложным (избыточным). Его можно сократить до:

  QDockAreaLayoutItem new_item    = widgetItem == nullptr      ? QDockAreaLayoutItem(subinfo) : QDockAreaLayoutItem(widgetItem);

Фрагмент N77: "код с запахом" избыточная защита

typedef unsigned int uint;ReturnedValue TypedArrayCtor::virtualCallAsConstructor(....){  ....  qint64 l = argc ? argv[0].toIndex() : 0;  if (scope.engine->hasException)    return Encode::undefined();  // ### lift UINT_MAX restriction  if (l < 0 || l > UINT_MAX)    return scope.engine->throwRangeError(QLatin1String("Index out of range."));  uint len = (uint)l;  if (l != len)    scope.engine->throwRangeError(      QStringLiteral("Non integer length for typed array."));  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'l != len' is always false. qv4typedarray.cpp 306

Кто-то очень переживает, что значение 64-битной переменной не вмещается в 32-битную переменную unsigned. И использует сразу две проверки корректности. При этом вторая проверка избыточна.

Вот этого условия более чем достаточно:

if (l < 0 || l > UINT_MAX)

Приведенный ниже фрагмент можно смело удалить, и программа менее надёжной не станет:

uint len = (uint)l;if (l != len)  scope.engine->throwRangeError(    QStringLiteral("Non integer length for typed array."));

Дальше продолжать не буду. Думаю, идею вы поняли.

Здесь можно сделать маленький вывод: результатом использования анализатора PVS-Studio будет не только устранение ошибок, но и упрощение кода.

Другие ошибки

Я остановился после того, как описал 77 дефектов. Это красивое число и выписанного более чем достаточно, чтобы написать статью. Однако это не значит, что нет других ошибок, которые способен выявить PVS-Studio. При изучении лога я был весьма поверхностен и пропускал всё, где нужно было разбираться более пары минут, ошибка это или нет :).

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

Заключение

Статический анализ это круто! После внедрения PVS-Studio будет экономить время и нервы, выявляя множество ошибок сразу после написания кода. Намного лучше искать на code review с коллегами не опечатки, а высокоуровневые ошибки и обсуждать эффективность реализованного алгоритма. Тем более, как показывает практика, эти дурацкие опечатки всё равно отлично прячутся при просмотре кода глазами. Так что пусть их лучше ищет программа, а не человек.

Если у вас ещё остались вопросы или возражения, приглашаю познакомиться со статьёй "Причины внедрить в процесс разработки статический анализатор кода PVS-Studio". С вероятность 90 % вы найдете в ней ответ на ваши вопросы :). В оставшихся 10 % случаев напишите нам, пообщаемся :).

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Date Processing Attracts Bugs or 77 Defects in Qt 6.

Хотя только недавно была заметка про проект CovidSim, есть хороший повод вновь про него вспомнить и продемонстрировать пользу регулярного использования PVS-Studio. Бывает, что все мы спешим и вносим правки в код, потеряв сосредоточенность. Статический анализатор может оказаться здесь хорошим помощником.

Всё началось с написания вот этих двух небольших заметок про открытый проект COVID-19 CovidSim Model:

• Пример, как в PVS-Studio появляются новые диагностики;
• Исследование COVID-19 и неинициализированная переменная.

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

Только мы начали проверять этот проект, как результаты не заставили себя ждать :). Сейчас я покажу свежую ошибку, вызванную недавними неаккуратными правками кода. Конечно, мы не станем описывать в дальнейшем каждый баг и недочёт, обнаруженный анализатором. Будем писать только про что-то интересное.

Вот что получилось, после недавних модификаций файла CovidSim.cpp:

Кто-то решил создавать массивы не на стеке, а в куче. Но был невнимателен, внося изменения. Обратите внимание, что освобождение памяти находится после оператора return:

int GetXMLNode(....){  char* buf = new char[65536];  char* CloseNode = new char[2048];  char* CloseParent = new char[2048];  ....  if (ResetFilePos) fseek(dat, CurPos, 0);  return ret;  delete[] buf;  delete[] CloseNode;  delete[] CloseParent;}

В результате перед нами фрагмент недостижимого кода (unreachable code). И заодно утечка памяти.

Хорошо, что PVS-Studio тут же сообщает про эту ошибку: V779 Unreachable code detected. It is possible that an error is present. CovidSim.cpp 675

Используйте статический анализ регулярно и сможете устранить множество ошибок на самом раннем этапе! Согласитесь, лучше сразу исправить этот мелкий баг, чем гадать, почему вдруг программа начала потреблять слишком много оперативной памяти.

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

Правильный и надёжный вариант кода:

std::unique_ptr<char[]> buf(new char[65536]);std::unique_ptr<char[]> CloseNode(new char[2048]);std::unique_ptr<char[]> CloseParent(new char[2048]);

Спасибо за внимание. Следуйте за мной в мир С++ и багов :). Twitter. Facebook.