РБПО

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

Разработка безопасного программного обеспечения (РБПО) требует использования инструментов статического анализа кода в двух плоскостях. Стандарт "Разработка безопасного программного обеспечения" (как старый ГОСТ Р 56939-2016, так и новый ГОСТ Р 56939-2024) явно предписывает использовать инструменты статического анализа кода. Однако в статье хочется обсудить не плоскость "используйте анализаторы, потому что это нужно для безопасности", а, собственно, на примерах показать, почему это важно. Другими словами, давайте попробуем на практике посмотреть, как использование статического анализатора позволяет делать код качественнее, надёжнее и безопаснее.

Пользуясь случаем, приглашаю подписаться на мой Телеграмм канал "Бестиарий программирования", где я сейчас публикую большой цикл постов, посвящённых РБПО.

Продолжим исследовать проект TDengine, о котором я уже написал три небольших заметки про рефакторинг кода:

Учимся рефакторить код на примере багов в TDengine, часть 1: про колбасу.
Учимся рефакторить код на примере багов в TDengine, часть 2: макрос, пожирающий стек.
Учимся рефакторить код на примере багов в TDengine, часть 3: плата за лень.

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

TDengine is an open source, high-performance, cloud native time-series database optimized for Internet of Things (IoT), Connected Cars, and Industrial IoT. It enables efficient, real-time data ingestion, processing, and monitoring of TB and even PB scale data per day, generated by billions of sensors and data collectors.

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

Обычно мы проверяем только код проекта, отделяя сторонние библиотеки, которые он использует. Однако в этот раз я сознательно не буду производить такое отделение и проверю весь код, из которого построен проект TDengine. Я хочу, чтобы вы подумали над таким моментом:

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

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

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

Кстати, проверка сторонних компонентов является неотъемлемой частью при построении процесса разработки безопасного программного обеспечения (РБПО). В ГОСТ Р 71207—2024 (Разработка безопасного программного обеспечения) указано, что целью статического анализа является:

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

В ГОСТ Р 71207— 2024 (Статический анализ программного обеспечения) также говорится про анализ заимствованного кода:

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

8.2. Статический анализатор должен поддерживать анализ ПО с используемыми заимствованными компонентами целиком, при этом удовлетворяя требованиям 7.3—7.6, 8.3 и 8.4.

Примечание. Статический анализатор PVS-Studio разрабатывается в соответствии с требованиями, изложенными в ГОСТ Р 71207—2024, и позволяет выполнять анализ заимствованных компонент.

130 оттенков нулевых указателей

ГОСТ Р 71207—2024 перечисляет типы критических ошибок, которые с наибольшей вероятностью могут привести к дефектам в безопасности. К ним относится и разыменование нулевых указателей (п 6.5.а). Это неудивительно, поскольку такие ошибки приводят к неопределённому поведению (т.е. невозможно предсказать последствия их возникновения).

Даже если в конкретном случае разыменование нулевого указателя всегда приводит к аварийному завершению программы, это тоже критическая ситуация. Потенциально такую ошибку можно использовать для DoS-атаки уровня приложения, т.е. передавать такие данные, которые приводят к нарушению их обработки (падению сервиса). Заодно ещё и данные в базе теоретически можно неконсистентными сделать, аварийно завершая работу в неподходящий момент.

Разыменование нулевого указателя — это очень частая ошибка, и в этом смысле код проекта TDengine не стал исключением.

Ошибка N1. Ошибка выбора

Функция taosArrayGetLast может вернутьNULL:

void* taosArrayGetLast(const SArray* pArray) {
  if (pArray->size == 0) {
    terrno = TSDB_CODE_INVALID_PARA;
    return NULL;
  }

  return TARRAY_GET_ELEM(pArray, pArray->size - 1);
}

Автор следующего кода хотел это учесть, но у него не получилось.

static int32_t walInitWriteFile(SWal *pWal) {
  int64_t       fileFirstVer = -1;
  ....
  SWalFileInfo *pRet = taosArrayGetLast(pWal->fileInfoSet);
  if (pRet == NULL) {
    fileFirstVer = pWal->vers.lastVer + 1;
  }
  fileFirstVer = pRet->firstVer;
  ....
}

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

V519 The 'fileFirstVer' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 696, 698. walWrite.c 698
V1004 The 'pRet' pointer was used unsafely after it was verified against nullptr. Check lines: 695, 698. walWrite.c 698

Наверное, для исправления ситуации следует добавить else:

SWalFileInfo *pRet = taosArrayGetLast(pWal->fileInfoSet);
if (pRet == NULL) {
  fileFirstVer = pWal->vers.lastVer + 1;
} else {
  fileFirstVer = pRet->firstVer;
}

Ошибки N2–N6. Ошибки в обработчиках ошибок

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

int32_t ctgGetFetchName(SArray* pNames, SCtgFetch* pFetch, SName** ppName) {
  STablesReq* pReq = (STablesReq*)taosArrayGet(pNames, pFetch->dbIdx);
  if (NULL == pReq) {
    qError("fail to get the %dth tb in pTables, tbNum:%d",
           pFetch->tbIdx, (int32_t)taosArrayGetSize(pReq->pTables));
    return TSDB_CODE_CTG_INTERNAL_ERROR;
  }
  ....
}

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'pReq' might take place. ctgUtil.c 1769

Если указатель pReq является нулевым, то происходит его разыменование, чтобы распечатать количество элементов в таблице:

STablesReq* pReq = ;
if (NULL == pReq) {
  ....(pReq->pTables));

Так себе идея :)

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

С другой стороны, она страшная:

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

Аналогичные дефекты в обработчиках ошибок:

V522 Dereferencing of the null pointer 'pBufInfo' might take place. groupcacheoperator.c 391
V522 Dereferencing of the null pointer 'item' might take place. scanoperator.c 4756
V522 Dereferencing of the null pointer 'pTrans' might take place. mndCompact.c 710
V522 Dereferencing of the null pointer 'pEntry' might take place. syncPipeline.c 885

Ошибки N7–N10. Неправильное использование assert

template <typename It>
static void
linkResultDirectedEdges(It first, It last)
// throw(TopologyException);
{
  for(; first != last; ++first) {
    Node* node = *first;
    assert(node);

    EdgeEndStar* ees = node->getEdges();
    assert(ees);
    DirectedEdgeStar* des = dynamic_cast<DirectedEdgeStar*>(ees);
    assert(des);

    // this might throw an exception
    des->linkResultDirectedEdges();
  }
}

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'des'. PlanarGraph.h 98

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

Утверждение (assert) предназначено, чтобы в процессе тестирования приложения убедиться, что данные лежат в ожидаемых диапазонах. Но в этом случае, используя dynamic_cast, программист подразумевает, что приведение объекта может не выполниться, и указатель окажется нулевым. Другими словами, нулевой указатель — это естественный вариант. Если же программист уверен, что приведение типа всегда должно выполняться успешно, следовало использовать static_cast. См. также статью на родственную тему: "Почему проверять результат вызова malloc c помощью assert плохая идея".

Следует заменить assert на оператор if и написать код, обрабатывающий ситуацию, когда указатель нулевой.

Другие аналогичные ошибки:

V522 There might be dereferencing of a potential null pointer 'nextedge'. LineMergeDirectedEdge.cpp 64
V522 There might be dereferencing of a potential null pointer 'edge'. EdgeRing.cpp 225
V522 There might be dereferencing of a potential null pointer 'point'. PointGeometryUnion.cpp 52

Ошибки N11–N13. Ещё более смелый dynamic_cast

Здесь вообще нет никакой проверки после выполнения dynamic_cast. Нулевой указатель может быть разыменован в условии при вызове функции nextedge->getEdgeDirection().

LineMergeDirectedEdge*
LineMergeDirectedEdge::getNext(bool checkDirection)
{
  ....
  if(getToNode()->getOutEdges()->getEdges()[0] == getSym()) {
    auto nextedge = dynamic_cast<LineMergeDirectedEdge*>(
      getToNode()->getOutEdges()->getEdges()[1]);
    return (!checkDirection || nextedge->getEdgeDirection()) ?
      nextedge : nullptr;
  }
  ....
}

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'nextedge'. LineMergeDirectedEdge.cpp 57

Результат dynamic_cast нужно проверять. Если же приведение типа обязано выполниться корректно в любом случае, то следует использовать более быстрый оператор static_cast. Это добавит ясности тем, кто будет сопровождать код.

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

auto nextedge = dynamic_cast<LineMergeDirectedEdge*>(
  getToNode()->getOutEdges()->getEdges()[1]);
return (!checkDirection ||
        (nextedge && nextedge->getEdgeDirection())) ?
  nextedge : nullptr;

Однако такой код выглядит слишком сложным для восприятия, а поэтому напишем менее плотный код:

auto nextedge = dynamic_cast<LineMergeDirectedEdge*>(
  getToNode()->getOutEdges()->getEdges()[1]);

if (!checkDirection ||
    (nextedge && nextedge->getEdgeDirection()))
{
  return nextedge;
}
return nullptr;

Другие предупреждения:

V522 There might be dereferencing of a potential null pointer. EdgeRing.cpp 300
V522 There might be dereferencing of a potential null pointer. EdgeRing.cpp 318

Ошибка N14. Макросы...

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

int32_t qWorkerInit(....) {
  ....
  if (NULL == mgmt->schHash) {
    taosMemoryFreeClear(mgmt);
    qError("init %d scheduler hash failed", mgmt->cfg.maxSchedulerNum);
    QW_ERR_JRET(terrno);
  }
  ....
}

Видите ошибку? Думаю, нет. Рассматривая этот код, сложно заметить, что здесь не так.

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

#define taosMemoryFreeClear(ptr)   \
  do {                             \
    if (ptr) {                     \
      taosMemoryFree((void *)ptr); \
      (ptr) = NULL;                \
    }                              \
  } while (0)

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

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'mgmt' might take place. qworker.c 1442

Чтобы исправить код, следует поместить вызов функции qError до макроса.

if (NULL == mgmt->schHash) {
  qError("init %d scheduler hash failed", mgmt->cfg.maxSchedulerNum);
  taosMemoryFreeClear(mgmt);
  QW_ERR_JRET(terrno);
}

Ох уж эти макросы.

Ошибки N15–N112. Отсутствие проверок при выделении памяти

В проекте TDengine достаточно безалаберно относятся к проверкам, удалось ли выделить память с помощью функции malloc (или аналогичных функций). Местами проверки есть:

void* buf = taosMemoryMalloc(tlen);
if (NULL == buf) {
  taosArrayDestroy(reqNew.pArray);
  tDeleteSVCreateTbBatchReq(&req);
  goto end;
}

Однако часто их нет. Это весьма печально и плохо, учитывая, что это библиотека для IoT устройств:

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

Более подробно всё это я разбирал в статье "Четыре причины проверять, что вернула функция malloc". Если кто-то у вас в команде не проверяет указатели после malloc, то предлагаю заставлять читать эту статью по кругу до полного осознания и просветления.

Как выглядит отсутствие проверок в TDengine? Одновременно разнообразно, но при этом скучно для рассмотрения. Приведу только пару примеров.

taos_linked_list_t *taos_linked_list_new(void) {
  taos_linked_list_t *self =
    (taos_linked_list_t *)taos_malloc(sizeof(taos_linked_list_t));
  self->head = NULL;
  self->tail = NULL;
  self->free_fn = NULL;
  self->compare_fn = NULL;
  self->size = 0;
  return self;
}

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'self'. Check lines: 28, 27. taos_linked_list.c 28

unsigned char*
SZ_skip_compress_double(double* data, size_t dataLength, size_t* outSize)
{
  *outSize = dataLength*sizeof(double);
  unsigned char* out = (unsigned char*)malloc(dataLength*sizeof(double));
  memcpy(out, data, dataLength*sizeof(double));
  return out;
}

Предупреждение PVS-Studio: V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 28, 27. sz_double.c 28

Другие аналогичные предупреждения.

V522 There might be dereferencing of a potential null pointer '* coeff_array'. Check lines: 304, 303. dataCompression.c 304
V522 There might be dereferencing of a potential null pointer 'keys'. Check lines: 344, 333. iniparser.c 344
V522 Dereferencing of the null pointer 'vce' might take place. The potential null pointer is passed into 'compressSingleFloatValue' function. Inspect the first argument. Check lines: 209, 439, 433. dataCompression.c 209
V522 Dereferencing of the null pointer 'lce' might take place. The potential null pointer is passed into 'addExactData' function. Inspect the fourth argument. Check lines: 275, 442, 434. dataCompression.c 275
V522 There might be dereferencing of a potential null pointer '* decData'. Check lines: 514, 463. dataCompression.c 514
V522 Dereferencing of the null pointer 'vce' might take place. The potential null pointer is passed into 'compressSingleDoubleValue' function. Inspect the first argument. Check lines: 234, 575, 569. dataCompression.c 234
V522 Dereferencing of the null pointer 'lce' might take place. The potential null pointer is passed into 'addExactData' function. Inspect the fourth argument. Check lines: 275, 578, 570. dataCompression.c 275
V522 There might be dereferencing of a potential null pointer 'result'. Check lines: 143, 134. CompressElement.c 143
V522 There might be dereferencing of a potential null pointer 'type'. Check lines: 152, 122. szd_double.c 152
V522 There might be dereferencing of a potential null pointer '* dia'. Check lines: 18, 17. DynamicIntArray.c 18
V522 There might be dereferencing of a potential null pointer 'type'. Check lines: 130, 107. sz_double.c 130
V522 There might be dereferencing of a potential null pointer 'vce'. Check lines: 132, 126. sz_double.c 132
V522 There might be dereferencing of a potential null pointer 'types'. Check lines: 160, 129. szd_float.c 160
V522 There might be dereferencing of a potential null pointer 'type2code'. Check lines: 48, 43. transcode.c 48
V522 There might be dereferencing of a potential null pointer 'diff'. Check lines: 49, 44. transcode.c 49
V522 There might be dereferencing of a potential null pointer 'tp_code'. Check lines: 63, 36. transcode.c 63
V522 There might be dereferencing of a potential null pointer 'tp_code'. Check lines: 146, 106. transcode.c 146
V522 There might be dereferencing of a potential null pointer 'type'. Check lines: 138, 114. sz_float.c 138
V522 There might be dereferencing of a potential null pointer 'vce'. Check lines: 141, 134. sz_float.c 141
V522 There might be dereferencing of a potential null pointer '* dba'. Check lines: 18, 17. DynamicByteArray.c 18
V522 There might be dereferencing of a potential null pointer 'huffmanTree->code[n->c]'. Check lines: 129, 125. Huffman.c 129
V522 There might be dereferencing of a potential null pointer '* out'. Check lines: 425, 424. Huffman.c 425
V522 There might be dereferencing of a potential null pointer 'symbol'. Check lines: 633, 632. dumper.c 633
V522 There might be dereferencing of a potential null pointer 'stackTrace'. Check lines: 716, 715. dumper.c 716
V522 There might be dereferencing of a potential null pointer 'subgeomArray'. Check lines: 2084, 2082. geos_ts_c.cpp 2084
V522 There might be dereferencing of a potential null pointer '* vgroup_ids'. Check lines: 97, 83. taos_counter.c 97
V522 There might be dereferencing of a potential null pointer '* keys'. Check lines: 98, 88. taos_counter.c 98
V522 There might be dereferencing of a potential null pointer 'node'. Check lines: 92, 90. taos_linked_list.c 92
V522 There might be dereferencing of a potential null pointer 'node'. Check lines: 108, 106. taos_linked_list.c 108
V522 There might be dereferencing of a potential null pointer 'self'. Check lines: 42, 41. taos_map.c 42
V522 There might be dereferencing of a potential null pointer 'self'. Check lines: 78, 77. taos_map.c 78
V522 There might be dereferencing of a potential null pointer 'self->addrs'. Check lines: 98, 94. taos_map.c 98
V522 There might be dereferencing of a potential null pointer 'new_addrs'. Check lines: 287, 283. taos_map.c 287
V522 There might be dereferencing of a potential null pointer 'self'. Check lines: 35, 34. taos_metric_formatter.c 35
V522 There might be dereferencing of a potential null pointer 'k'. Check lines: 60, 47. taos_metric.c 60
V522 There might be dereferencing of a potential null pointer 'self'. Check lines: 45, 44. taos_string_builder.c 45
V522 There might be dereferencing of a potential null pointer 'self->str'. Check lines: 59, 58. taos_string_builder.c 59
V522 There might be dereferencing of a potential null pointer 'self'. Check lines: 49, 47. taos_collector_registry.c 49
V522 There might be dereferencing of a potential null pointer 'self'. Check lines: 39, 38. taos_metric_sample.c 39
V522 There might be dereferencing of a potential null pointer 'e'. Check lines: 532, 530. lru_cache.cc 532
V522 There might be dereferencing of a potential null pointer 'column_families'. Check lines: 1038, 1036. c.cc 1038
V522 There might be dereferencing of a potential null pointer 'cf_names'. Check lines: 2526, 2522. c.cc 2526
V522 There might be dereferencing of a potential null pointer 'cf_options'. Check lines: 2527, 2523. c.cc 2527
V522 There might be dereferencing of a potential null pointer 'level_meta'. Check lines: 5308, 5307. c.cc 5308
V522 There might be dereferencing of a potential null pointer 'file_meta'. Check lines: 5339, 5338. c.cc 5339
V522 There might be dereferencing of a potential null pointer 'buf'. Check lines: 5599, 5596. c.cc 5599
V522 There might be dereferencing of a potential null pointer 'wi'. Check lines: 5627, 5626. c.cc 5627
V522 There might be dereferencing of a potential null pointer 'result'. Check lines: 5672, 5671. c.cc 5672
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 51, 50. sz_double.c 51
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 273, 272. sz_double.c 273
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 380, 379. sz_double.c 380
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 47, 46. sz_float.c 47
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 275, 274. sz_float.c 275
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 402, 401. sz_float.c 402
V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 33, 27. DynamicByteArray.c 33
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 20, 19. Huffman.c 20
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 29, 24. Huffman.c 29
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 30, 25. Huffman.c 30
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 31, 26. Huffman.c 31
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 32, 27. Huffman.c 32
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 172, 171. Huffman.c 172
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 194, 193. Huffman.c 194
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 413, 412. Huffman.c 413
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 415, 414. Huffman.c 415
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 417, 416. Huffman.c 417
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 419, 418. Huffman.c 419
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 440, 439. Huffman.c 440
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 442, 441. Huffman.c 442
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 444, 443. Huffman.c 444
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 446, 445. Huffman.c 446
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 464, 463. Huffman.c 464
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 466, 465. Huffman.c 466
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 468, 467. Huffman.c 468
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 470, 469. Huffman.c 470
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 566, 565. Huffman.c 566
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 568, 567. Huffman.c 568
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 570, 569. Huffman.c 570
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 572, 571. Huffman.c 572
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 603, 602. Huffman.c 603
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 605, 604. Huffman.c 605
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 607, 606. Huffman.c 607
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 609, 608. Huffman.c 609
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 656, 655. Huffman.c 656
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 658, 657. Huffman.c 658
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 660, 659. Huffman.c 660
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 662, 661. Huffman.c 662
V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 710, 708. Huffman.c 710
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 22, 21. TightDataPointStorageF.c 22
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 194, 193. TightDataPointStorageF.c 194
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 22, 21. TightDataPointStorageD.c 22
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 194, 193. TightDataPointStorageD.c 194
V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 3320, 3319. geos_ts_c.cpp 3320
V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 3337, 3336. geos_ts_c.cpp 3337
V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 42, 41. taos_metric.c 42
V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 145, 144. taos_string_builder.c 145
V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 533, 532. c.cc 533

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

Ошибки N113–N130 (на самом деле больше). Разыменование указателя до его проверки.

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

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

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

bool
RectangleIntersection::clip_linestring_parts(
  const geom::LineString* gi, ....)
{
  auto n = gi->getNumPoints();

  if(gi == nullptr || n < 1) {
    return false;
  }
  ....
}

Предупреждение PVS-Studio: V595 The 'gi' pointer was utilized before it was verified against nullptr. Check lines: 137, 139. RectangleIntersection.cpp 137

Проверка указателя gi явно запоздала. Думаю, подробнее останавливаться на этом случае смысла нет.

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

int32_t tsortOpen(SSortHandle* pHandle) {
  int32_t code = 0;
  if (pHandle->opened) {
    return code;
  }

  if (pHandle == NULL || pHandle->fetchfp == NULL ||
      pHandle->comparFn == NULL) {
    return TSDB_CODE_INVALID_PARA;
  }
  ....
}

V595 The 'pHandle' pointer was utilized before it was verified against nullptr. Check lines: 2883, 2887. tsort.c 2883

Тут всё то же самое. Бывают некоторые вариации, но, надеюсь, смысл понятен. Если же логика работы диагностики V595 показалась вам не совсем очевидной, то предлагаю ознакомиться со следующей заметкой: "Пояснение про диагностику V595". Заметка написана 10 лет назад, и за это время анализатор научился куда более глубоко анализировать потоки данных и вычислять возможные значения указателей. Однако это не делает диагностику менее полезной, и она по-прежнему эффективно находит баги, связанные с путаницей в последовательности разыменования и проверки указателей.

Другие ошибки данного типа.

V595 The 'col' pointer was utilized before it was verified against nullptr. Check lines: 2075, 2076. geos_ts_c.cpp 2075
V595 The 'keys' pointer was utilized before it was verified against nullptr. Check lines: 88, 89. taos_counter.c 88
V595 The 'dbCache' pointer was utilized before it was verified against nullptr. Check lines: 196, 199. ctgCache.c 196
V595 The 'pDbCache' pointer was utilized before it was verified against nullptr. Check lines: 1631, 1633. ctgCache.c 1631
V595 The 'pInfo->pState' pointer was utilized before it was verified against nullptr. Check lines: 155, 158. streamfilloperator.c 155
V595 The 'pFillInfo' pointer was utilized before it was verified against nullptr. Check lines: 1582, 1588. streamfilloperator.c 1582
V595 The 'string' pointer was utilized before it was verified against nullptr. Check lines: 869, 870. mndTopic.c 869
V595 The 'pFile' pointer was utilized before it was verified against nullptr. Check lines: 1351, 1353. osFile.c 1351
V595 The 'bins' pointer was utilized before it was verified against nullptr. Check lines: 4267, 4268. sclfunc.c 4267
V595 The 'pCtx->freeFunc' pointer was utilized before it was verified against nullptr. Check lines: 351, 358. schUtil.c 351
V595 The 'pNodeList' pointer was utilized before it was verified against nullptr. Check lines: 481, 482. clientImpl.c 481
V595 The 'pReq' pointer was utilized before it was verified against nullptr. Check lines: 2831, 2832. clientImpl.c 2831
V595 The 'vgroup_ids' pointer was utilized before it was verified against nullptr. Check lines: 83, 84. taos_counter.c 83
V595 The 'pReq' pointer was utilized before it was verified against nullptr. Check lines: 1214, 1215. transCli.c 1214
V595 The 'pReq' pointer was utilized before it was verified against nullptr. Check lines: 2755, 2758. transCli.c 2755
V595 The 'pReq' pointer was utilized before it was verified against nullptr. Check lines: 3364, 3380. transCli.c 3364
Здесь мне стало скучно и прекратил выписывать предупреждения. На самом деле ошибок больше.

Утечка ресурсов

Ручное управление памятью чревато ошибками. Проект TDengine в основном написан на C, так что неудивительно, что и в нём мы можем встретить такие ошибки.

ГОСТ Р 71207—2024 причисляет утечки памяти, незакрытие файловых дескрипторов и дескрипторов сетевых соединений к критическим ошибкам (п. 6.5.е). Например, Анна Мелихова из Лаборатории Касперского в докладе "Низкоуровневая безопасность: нужно, можно, не рекомендуем" рассказывает, как возникает потенциальная уязвимость из-за того, что на одном из путей выполнения программы забыли закрыть файловый дескриптор.

Ошибки N131–N140. Утечка памяти на одном из путей выполнения программы

taos_map_t *taos_map_new() {
  int r = 0;

  taos_map_t *self = (taos_map_t *)taos_malloc(sizeof(taos_map_t));
  self->size = 0;
  self->max_size = TAOS_MAP_INITIAL_SIZE;

  self->keys = taos_linked_list_new();
  if (self->keys == NULL) return NULL;
  ....
}

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

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

Схожие ошибки.

V773 The function was exited without releasing the 'new_addrs' pointer. A memory leak is possible. taos_map.c 289
V773 The function was exited without releasing the 'k' pointer. A memory leak is possible. taos_metric.c 52
V773 The function was exited without releasing the 'new_nexts' pointer. A memory leak is possible. regex_internal.c 1421
V773 The function was exited without releasing the 'new_indices' pointer. A memory leak is possible. regex_internal.c 1421
V773 The function was exited without releasing the 'new_edests' pointer. A memory leak is possible. regex_internal.c 1421
V773 The function was exited without releasing the 'new_eclosures' pointer. A memory leak is possible. regex_internal.c 1421
V773 The function was exited without releasing the 'self' pointer. A memory leak is possible. taos_collector_registry.c 53
V773 The function was exited without releasing the 'new_start' pointer. A memory leak is possible. regexec.c 534
V773 The function was exited without releasing the 'new_end' pointer. A memory leak is possible. regexec.c 534

Ошибки N141–N144. Неаккуратное использование функции realloc

INLINE void addDIA_Data(DynamicIntArray *dia, int value)
{
  if(dia->size==dia->capacity)
  {
    dia->capacity = dia->capacity << 1;
    dia->array = (unsigned char *)
      realloc(dia->array, dia->capacity*sizeof(unsigned char));
  }
  dia->array[dia->size] = (unsigned char)value;
  dia->size ++;
}

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

Если функция realloc не сможет выделить новый буфер, то она вернёт NULL. Старое значение указателя dia->array будет потеряно и невозможно будет освободить буфер, адрес которого ранее в нём хранился.

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

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

dia->array[dia->size] = (unsigned char)value;

Будет осуществляться не по нулевому указателю, а в какой-то отдалённый участок памяти, адрес которого зависит от предыдущего размера массива. Последствия непредсказуемы. Возможен Access Violation. А возможно, будут испорчены какие-то данные в памяти, и программа при этом продолжит работу, по крайней мере на какое-то время. Как говорится, счастливой отладки :)

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

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'dba->array' is lost. Consider assigning realloc() to a temporary pointer. DynamicByteArray.c 57
V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'dba->array' is lost. Consider assigning realloc() to a temporary pointer. DynamicByteArray.c 68
V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'self->str' is lost. Consider assigning realloc() to a temporary pointer. taos_string_builder.c 84

Ошибки N145–N157. Неаккуратное использование функции emplace_back

Status SstFileWriter::Open(const std::string& file_path) {
  ....
  for (size_t i = 0; i < user_collector_factories.size(); i++) {
    int_tbl_prop_collector_factories.emplace_back(
        new UserKeyTablePropertiesCollectorFactory(
            user_collector_factories[i]));
  }
  ....
}

Предупреждение PVS-Studio: V1023 A pointer without owner is added to the 'int_tbl_prop_collector_factories' container by the 'emplace_back' method. A memory leak will occur in case of an exception. sst_file_writer.cc 298

Если контейнер полон, то происходит перевыделение памяти. Эта операция может закончиться неудачей, в результате чего будет сгенерировано исключение std::bad_alloc. В этом случае указатель будет потерян, и созданный объект никогда не будет удалён.

Безопасная конструкция, защищающая от потенциальной утечки памяти:

int_tbl_prop_collector_factories.emplace_back(
  std::make_unique<UserKeyTablePropertiesCollectorFactory>(
    user_collector_factories[i]));

Другие предупреждения.

V1023 A pointer without owner is added to the 'locations' container by the 'emplace_back' method. A memory leak will occur in case of an exception. ConnectedElementLocationFilter.cpp 54
V1023 A pointer without owner is added to the 'locations' container by the 'emplace_back' method. A memory leak will occur in case of an exception. ConnectedElementLocationFilter.cpp 67
V1023 A pointer without owner is added to the 'outOERs' container by the 'emplace_back' method. A memory leak will occur in case of an exception. MaximalEdgeRing.cpp 117
V1023 A pointer without owner is added to the 'edgeRings' container by the 'emplace_back' method. A memory leak will occur in case of an exception. PolygonBuilder.cpp 87
V1023 A pointer without owner is added to the 'copied_operands_' container by the 'emplace_back' method. A memory leak will occur in case of an exception. merge_context.h 41
V1023 A pointer without owner is added to the 'copied_operands_' container by the 'emplace_back' method. A memory leak will occur in case of an exception. merge_context.h 57
V1023 A pointer without owner is added to the 'jobs' container by the 'emplace_back' method. A memory leak will occur in case of an exception. db_impl_compaction_flush.cc 458
V1023 A pointer without owner is added to the 'parent_iters_' container by the 'emplace_back' method. A memory leak will occur in case of an exception. range_del_aggregator.cc 373
V1023 A pointer without owner is added to the 'builder_guards' container by the 'emplace_back' method. A memory leak will occur in case of an exception. version_set.cc 5026
V1023 A pointer without owner is added to the 'table_properties_collectors' container by the 'emplace_back' method. A memory leak will occur in case of an exception. block_based_table_builder.cc 533
V1023 A pointer without owner is added to the 'table_properties_collectors' container by the 'emplace_back' method. A memory leak will occur in case of an exception. block_based_table_builder.cc 540
V1023 A pointer without owner is added to the 'int_tbl_prop_collector_factories' container by the 'emplace_back' method. A memory leak will occur in case of an exception. sst_file_writer.cc 290

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

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

Думаю, ожидаемо, что ошибки переполнения буфера согласно ГОСТ Р 71207—2024 также причислены к критическим (п. 6.3.в.). Если вы планируете построить процесс разработки безопасного программного обеспечения (РБПО), то используйте инструментальные средства, которые позволяют выявлять все виды критических ошибок. Одним из них является статический анализатор PVS-Studio.

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

Андрей Карпов. ГОСТ Р 71207–2024 — Статический анализ программного обеспечения. Критические ошибки.

Ошибка N158. Выход за границу буфера

const char* rocksdb_iter_value(const rocksdb_iterator_t* iter, size_t* vlen) {
  Slice s = iter->rep->value();
  *vlen = s.size();
  return s.data();
}

int32_t streamDefaultIterGet_rocksdb(....) {
  ....
  while (rocksdb_iter_valid(pIter)) {
    const char* key = rocksdb_iter_key(pIter, &klen);
    int32_t     vlen = 0;
    const char* vval = rocksdb_iter_value(pIter, (size_t*)&vlen);
  ....
}

Предупреждение PVS-Studio: V512 A call of the 'rocksdb_iter_value' function will lead to overflow of the buffer '& vlen'. streamBackendRocksdb.c 4390

Этот код будет работать, если собрать 32-битную версию приложения. На 64-битной сборке он даст сбой.

Адрес 32-битной переменной vlen интерпретируется как указатель на тип size_t:

int32_t     vlen = 0;
const char* vval = rocksdb_iter_value(pIter, (size_t*)&vlen);

В функции rocksdb_iter_value по этому адресу будет записано значение типа size_t.

В 32-битной программе (если не рассматривать экзотические архитектуры) размер переменной size_t равен 4 байтам и совпадает с размером типа int32_t. Код будет работать корректно.

В 64-битной программе размер типа size_t равен 8 байтам. Соответственно, при записи 64-битного значения по адресу переменной vlen часть данных будет записана за пределами этой переменной. В общем, 4 байта будут записаны в стек после этой переменной, что приведёт к неопределённому поведению программы.

Ошибка N159. Выход за границу буфера из-за путаницы в константах

Вначале запомним, что MAX_QUERY_VALUE_LEN это 1024:

#define MAX_QUERY_VALUE_LEN       1024

Далее запомним, что третья размерность массива char data[100][100][1024] это тоже 1024:

typedef struct _script_t {
  ....
  char              cols[12];
  char              data[100][100][1024];
  char              system_exit_code[12];
  ....
} SScript;

Наконец, собственно, код с ошибкой:

bool simExecuteNativeSqlCommand(SScript *script, char *rest, bool isSlow) {
  ....
  char *value = NULL;
  if (i < MAX_QUERY_COL_NUM) {
    value = script->data[numOfRows][i];
  }
  if (value == NULL) {
    continue;
  }
  ....
  int32_t    *length = taos_fetch_lengths(pSql);
  ....
  if (length[i] < 0 || length[i] > 1 << 20) {
    fprintf(stderr, "Invalid length(%d) of BINARY or NCHAR\n", length[i]);
    exit(-1);
  }
  memset(value, 0, MAX_QUERY_VALUE_LEN);
  memcpy(value, row[i], length[i]);
  value[length[i]] = 0;  
  ....
}

Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to overflow of the buffer 'value'. simExec.c 786

Есть некое значение, лежащее в ячейке массива length[i]. Оно предварительно проверяется:

if (length[i] < 0 || length[i] > 1 << 20) {
  fprintf(stderr, "Invalid length(%d) of BINARY or NCHAR\n", length[i]);
  exit(-1);
}

Затем это значение используется как размер копируемых данных:

memcpy(value, row[i], length[i]);

Проблема в том, что 1 << 20 это вовсе не 1024, а 1048576. Соответственно, проверка не защищает от потенциального переполнения буфера. Думаю, верным будет использовать в условии не магическое число, а именованную константу MAX_QUERY_VALUE_LEN. Эту константу следует использовать и при объявлении массива.

char        data[100][100][MAX_QUERY_VALUE_LEN];  
....
if (length[i] < 0 || length[i] > MAX_QUERY_VALUE_LEN) {
  fprintf(stderr, "Invalid length(%d) of BINARY or NCHAR\n", length[i]);
  exit(-1);
}
memset(value, 0, MAX_QUERY_VALUE_LEN);
memcpy(value, row[i], length[i]);

Ой, сейчас понял, что в коде ещё одна ошибка есть:

value[length[i]] = 0;

Эта строчка не нужна и даже вредна. Во-первых, массив предварительно уже заполнен нулями после вызова функции memset и дополнительный терминальный ноль не нужен. Во-вторых, если length[i] == MAX_QUERY_VALUE_LEN, то произойдёт выход за границу массива. В общем, эту строчку надо просто удалить.

Продолжим размышления. Раз предполагается терминальный ноль в конце, нельзя копировать MAX_QUERY_VALUE_LEN байт. Тогда не останется места под терминальный ноль. Следовательно, надо ещё раз модифицировать проверку и использовать оператор >=, а не >.

if (length[i] < 0 || length[i] >= MAX_QUERY_VALUE_LEN)

Ошибка N160. Потенциальный выход за границу массива

Минутка внимания. Вы осознаёте, что мы уже добрались до 160 ошибки?!

160 ошибок

Осознайте, что мы нашли уже 160 ошибок в базе данных. Это прискорбно.

Причём согласно ГОСТ Р 71207—2024 всё рассмотренное выше — это критические ошибки, которые должны оперативно находиться и исправляться. А не вот так кучей, возможно годами, валяться в коде проекта.

ГОСТ Р 71207—2024 (п. 5.9). Выявленные потенциальные ошибки, которые в результате верификации согласно 5.7 были отнесены к истинным, не позже, чем через 10 рабочих дней после выполнения разметки следует исправить либо запланировать сроки их устранения в соответствии с принятыми разработчиком ПО процессами разработки ПО. Если применяется гибкая методология разработки ПО, то план по устранению потенциальной ошибки включается в ближайший цикл разработки. При анализе ПО целиком устранение выявленных потенциальных критических ошибок должно быть выполнено не позднее начала квалификационного тестирования ПО.

Прописываю проекту TDengine немедленное внедрение статических анализаторов кода в процесс разработки :)

dictionary * iniparser_load(const char * ininame)
{
  ....
  char line    [ASCIILINESZ+1] ;
  ....
  memset(line,    0, ASCIILINESZ);
  ....
  last=0 ;

  while (fgets(line+last, ASCIILINESZ-last, in)!=NULL) {
    lineno++ ;
    len = (int)strlen(line)-1;
    if (len==0)
      continue;
    /* Safety check against buffer overflows */
    if (line[len]!='\n') {
      fprintf(stderr,
              "iniparser: input line too long in %s (%d)\n",
              ininame,
              lineno);
      dictionary_del(dict);
      fclose(in);
      return NULL ;
    }
    ....
}

Предупреждение PVS-Studio: V557 Array underrun is possible. The value of 'len' index could reach -1. iniparser.c 695

Если входными данными для fgets будет пустая строка, то переменная len окажется равна -1. Как следствие, выход за границу массива. Как именно всё будет происходить, мы уже ранее разбирали в статье "Стреляем в ногу, обрабатывая входные данные". Там рассматриваются идентичные воспроизводимые ошибки.

Учитывая всё это, забавно смотрится комментарий в коде:

/* Safety check against buffer overflows */

Опечатки

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

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

Ошибка N161. Перезапись значения

static int32_t getRowsBlockWithinMergeLimit(....) {
  ....
  if (keepRows == 0) {
    *pSkipBlock = true;
    *pRes = pOrigBlk;
  }

  *pSkipBlock = false;
  ....
}

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

Строчка *pSkipBlock = true не имеет смысла, так как потом в любом случае значение поменяется на false. Скорее всего, требуется добавить else:

if (keepRows == 0) {
  *pSkipBlock = true;
  *pRes = pOrigBlk;
}
else {
  *pSkipBlock = false;
}

Ошибка N162. Повтор

static void processSimpleMeta(SMqMetaRsp* pMetaRsp, cJSON** meta) {
  ...
  } else if (pMetaRsp->resMsgType == TDMT_VND_ALTER_TABLE) {
    processAlterTable(pMetaRsp, meta);
  } else if (pMetaRsp->resMsgType == TDMT_VND_DROP_TABLE) {
    processDropTable(pMetaRsp, meta);
  } else if (pMetaRsp->resMsgType == TDMT_VND_DROP_TABLE) {
    processDropTable(pMetaRsp, meta);
  } else if (pMetaRsp->resMsgType == TDMT_VND_DELETE) {
  ....
}

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2316, 2318. clientRawBlockWrite.c 2316

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

} else if (pMetaRsp->resMsgType == TDMT_VND_DROP_TABLE) {
  processDropTable(pMetaRsp, meta);

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

Ошибка N163. Забытый опасный код, где возможно деление на 0

OffsetSegmentGenerator::OffsetSegmentGenerator(....) : ....
{
  ....
  // compute intersections in full precision, to provide accuracy
  // the points are rounded as they are inserted into the curve line
  filletAngleQuantum = MATH_PI / 2.0 / bufParams.getQuadrantSegments();

  int quadSegs = bufParams.getQuadrantSegments();
  if (quadSegs < 1) quadSegs = 1;
  filletAngleQuantum = MATH_PI / 2.0 / quadSegs;
  ....
}

Предупреждение PVS-Studio: V519 The 'filletAngleQuantum' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 82, 86. OffsetSegmentGenerator.cpp 86

Вначале был написан код:

filletAngleQuantum = MATH_PI / 2.0 / bufParams.getQuadrantSegments();

Функция getQuadrantSegments может вернуть 0, поэтому код был переписан, чтобы защититься от деления на ноль:

int quadSegs = bufParams.getQuadrantSegments();
if (quadSegs < 1) quadSegs = 1;
filletAngleQuantum = MATH_PI / 2.0 / quadSegs;

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

Ошибка N164. Одинаковые функции

typedef struct {
  int64_t firstVer;
  int64_t lastVer;
  int64_t createTs;
  int64_t closeTs;
  int64_t fileSize;
  int64_t syncedOffset;
} SWalFileInfo;

static inline int64_t walGetCurFileFirstVer(SWal* pWal) {
  if (pWal->writeCur == -1) return -1;
  SWalFileInfo* pInfo =
    (SWalFileInfo*)taosArrayGet(pWal->fileInfoSet, pWal->writeCur);
  return pInfo->firstVer;
}

static inline int64_t walGetCurFileLastVer(SWal* pWal) {
  if (pWal->writeCur == -1) return -1;
  SWalFileInfo* pInfo =
    (SWalFileInfo*)taosArrayGet(pWal->fileInfoSet, pWal->writeCur);
  return pInfo->firstVer;
}

Предупреждение PVS-Studio: V524 It is odd that the body of 'walGetCurFileLastVer' function is fully equivalent to the body of 'walGetCurFileFirstVer' function. walInt.h 97

В структуре SWalFileInfo есть два поля:

firstVer;
lastVer.

Есть две функции для получения значений из этих полей:

walGetCurFileFirstVer;
walGetCurFileLastVer.

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

Ошибки N165–N170. "Проклятие скобочек"

int32_t dmInit() {
  dInfo("start to init dnode env");
  int32_t code = 0;
  ....
  if ((code = dmCheckDiskSpace()) != 0) return code;
  if ((code = dmCheckRepeatInit(dmInstance())) != 0) return code;
  if ((code = dmInitSystem()) != 0) return code;
  if ((code = dmInitMonitor()) != 0) return code;
  if ((code = dmInitAudit()) != 0) return code;
  if ((code = dmInitDnode(dmInstance())) != 0) return code;
  if ((code = InitRegexCache() != 0)) return code;
  ....
}

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

Кто видит ошибку? ;)

На мой взгляд, красивая опечатка. В последней проверке скобочка не там поставлена. Результат вызова функции InitRegexCache сравнивается с 0, и только затем 0 или 1 записывается в переменную code.

Есть ещё несколько аналогичных опечаток:

V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. mndArbGroup.c 299
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. mndConfig.c 430
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. mndUser.c 418
V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. streamMeta.c 411
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. transCli.c 1876

Ошибка N171. Бессмысленная проверка

void
CoordinateSequence::add(const CoordinateSequence& cs,
                        std::size_t from, std::size_t to)
{
  if (cs.stride() == stride() && cs.hasM() == cs.hasM()) {
      m_vect.insert(m_vect.end(),
                    std::next(cs.m_vect.cbegin(),
                    static_cast<std::ptrdiff_t>(from * stride())),
                    std::next(cs.m_vect.cbegin(),
                    static_cast<std::ptrdiff_t>((to + 1u)*stride())));
  } else {
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: cs.hasM() == cs.hasM() CoordinateSequence.cpp 154

Рука программиста дрогнула, и он случайно дописал лишний cs.. В результате получается, что часть условия всегда будет истинной:

cs.hasM() == cs.hasM()

Ошибка N172. Не ошибка, но ошибка

bool startsWith(const std::string & s, char prefix) {
  if (s.empty() == 0) {
    return false;
  }

  return s[0] == prefix;
}

Предупреждение PVS-Studio: V557 Array overrun is possible. The '0' index is pointing beyond array bound. string.cpp 53

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

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

Примечание N2. До C++11 это именно ошибка, так как такой код вызывает неопределённое поведение.

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

bool startsWith(const std::string & s, char prefix) {
  if (s.empty()) {
    return false;
  }

  return s[0] == prefix;
}

Ошибка N173. Опечатка при использовании схожих имён переменных

int32_t streamTaskUpdateTaskCheckpointInfo(....) {
  ....
  bool valid = (pInfo->checkpointId  <= pReq->checkpointId &&
                pInfo->checkpointVer <= pReq->checkpointVer &&
                pInfo->processedVer  <= pReq->checkpointVer);
  ....
}

Предупреждение PVS-Studio: V1013 Suspicious subexpression in a sequence of similar comparisons. streamCheckpoint.c 654

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

Переменная pInfo->processedVer должна сравниваться не с pReq->checkpointVer, а с pReq->processedVer.

Ошибки N174 и N175. Колбаса и плата за лень

Ещё две опечатки я уже рассматривал в предыдущих статьях, посвящённых рефакторингу неаккуратного кода:

Учимся рефакторить код на примере багов в TDengine, часть 1: про колбасу.
Учимся рефакторить код на примере багов в TDengine, часть 3: плата за лень.

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

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

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

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

Ошибки N176 и N177. Ошибки использования оператора сдвига <<

uint64_t unpackUint64(uint8_t* ch, uint8_t sz) {
  uint64_t n = 0;
  for (uint8_t i = 0; i < sz; i++) {
    n = n | (ch[i] << (8 * i));
  }
  return n;
}

Предупреждение PVS-Studio: V629 Consider inspecting the 'ch[i] << (8 * i)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. indexFstUtil.c 55

Из массива байт должно быть создано 64-битное значение. Однако произойдёт сбой при попытке выставить старшие 32-бита в 64-битной переменной. При сдвиге левый операнд, который является 8-битным беззнаковым символом, будет неявно преобразован в 32-битный int. Однако этого недостаточно: при сдвиге значения int на более чем 31 разряд произойдёт переполнение. Необходимо явно выполнить предварительное преобразование к 64-битному типу:

n = n | ((uint64_t)(ch[i]) << (8 * i));

Схожая ошибка:

static int hashset_add(hashset_t set, void *item) {
  int ret = hashset_add_member(set, item);

  size_t old_capacity = set->capacity;
  if (set->nitems >= (double)old_capacity * set->load_factor) {
    size_t *old_items = set->items;
    ++set->nbits;
    set->capacity = (size_t)(1 << set->nbits);
  ....
}

Но предупреждение PVS-Studio другое: V1028 Possible overflow. Consider casting operands of the '1 << set->nbits' operator to the 'size_t' type, not the result. tdbPager.c 88

Ошибка N178. Недостижимый код

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

static int32_t getBlkFromSessionCache(struct SOperatorInfo* pOperator,
  int64_t sessionId, SGcSessionCtx* pSession, SSDataBlock** ppRes)
{
  int32_t code = TSDB_CODE_SUCCESS;
  SGroupCacheOperatorInfo* pGCache = pOperator->info;
  bool locked = false;
  SGcDownstreamCtx* pCtx = &pGCache->pDownstreams[pSession->downstreamIdx];

  while (true) {
    bool got = false;
    code = getBlkFromSessionCacheImpl(pOperator, sessionId,
                                      pSession, ppRes, &got);
    if (TSDB_CODE_SUCCESS != code || got) {
      goto _return;
    }

    if ((atomic_load_64(&pCtx->fetchSessionId) == sessionId)
      || (-1 == atomic_val_compare_exchange_64(
                  &pCtx->fetchSessionId, -1, sessionId))) {
      if (locked) {
        (void)taosThreadMutexUnlock(&pSession->pGroupData->mutex);
        locked = false;
      }

      code = getCacheBlkFromDownstreamOperator(pOperator, pCtx,
                                               sessionId, pSession, ppRes);
      goto _return;
    } else {
      // FOR NOW, SHOULD NOT REACH HERE
      qError("Invalid fetchSessionId:%" PRId64 ",
             currentSessionId:%" PRId64, pCtx->fetchSessionId, sessionId);
      return TSDB_CODE_QRY_EXECUTOR_INTERNAL_ERROR;
    }

    if (locked) {
      code = groupCacheSessionWait(pOperator, pCtx, sessionId,
                                   pSession, ppRes);
      locked = false;
      if (TSDB_CODE_SUCCESS != code) {
        goto _return;
      }

      break;
    }

    (void)taosThreadMutexLock(&pSession->pGroupData->mutex);
    locked = true;
  };

_return:

  if (locked) {
    (void)taosThreadMutexUnlock(&pSession->pGroupData->mutex);
  }

  return code;
}

Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. groupcacheoperator.c 1227

Удалось? Если нет, то вот где ошибка:

if (....)        // << (A)
{
  ....
  goto _return;  // << (B)
} else {
  ....
  return TSDB_CODE_QRY_EXECUTOR_INTERNAL_ERROR; // << (C)
}
....
if (locked) {    // << (D)
....
_return:         // << (E)

Независимо от условия (A), код (D) никогда не получит управление. Произойдёт или переход (B) к метке (E), или выход из функции ©.

Ошибки N179–N184. Потенциальное переполнение

typedef struct SFilePage {
  int32_t num;
  ....
} SFilePage;

typedef struct tMemBucket {
  ....
  int32_t            bytes;
  ....
} tMemBucket;

static int32_t loadDataFromFilePage(tMemBucket *pMemBucket, ....) {
  ....
  SFilePage *pg = getBufPage(pMemBucket->pBuffer, *pageId);
  ....
  (void)memcpy((*buffer)->data + offset, pg->data,
               (size_t)(pg->num * pMemBucket->bytes));
  ....
}

Предупреждение PVS-Studio: V1028 Possible overflow. Consider casting operands of the 'pg->num * pMemBucket->bytes' operator to the 'size_t' type, not the result. tpercentile.c 64

Явное приведение типа не помогает избежать переполнения при перемножении 32-битных переменных.

(size_t)(pg->num * pMemBucket->bytes)

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

(size_t)(pg->num) * pMemBucket->bytes

Схожие предупреждения:

V1028 Possible overflow. Consider casting operands of the 'pColData->nVal + 1' operator to the 'int64_t' type, not the result. tdataformat.c 1904
V1028 Possible overflow. Consider casting operands of the 'pColData->nVal + 1' operator to the 'int64_t' type, not the result. tdataformat.c 1904
V1028 Possible overflow. Consider casting operands, not the result. compaction_picker_level.cc 818
V1028 Possible overflow. Consider casting operands of the 'vlen * 4' operator to the 'size_t' type, not the result. tbase64.c 23
V1028 Possible overflow. Consider casting operands of the 'inlen * 3' operator to the 'size_t' type, not the result. tbase64.c 59

Ошибка N185. Недопустимое расширение пространства имён std

namespace std {
inline void swap(ROCKSDB_NAMESPACE::port::WindowsThread& th1,
                 ROCKSDB_NAMESPACE::port::WindowsThread& th2) {
  th1.swap(th2);
}
}  // namespace std

Предупреждение PVS-Studio: V1061 Extending the 'std' namespace may result in undefined behavior. win_thread.h 110

С описанием этого дефекта можно познакомиться в документации, а я уже устал описывать все эти баги :) Сейчас дойду до красивого числа в 190 багов и остановлюсь. Или до 200 дойти? Нет, долой перфекционизм, нужно отдохнуть.

Ошибка N186. Сравнение "мусорных" байтов

typedef struct STreeNode {
  int32_t index;
  void   *pData;  // TODO remove it?
} STreeNode;

int32_t tMergeTreeAdjust(SMultiwayMergeTreeInfo* pTree, int32_t idx) {
  ....
  STreeNode kLeaf = pTree->pNode[idx];
  ....
  if (memcmp(&kLeaf, &pTree->pNode[1], sizeof(kLeaf)) != 0) {
  ....
}

Предупреждение PVS-Studio: V1103 The values of padding bytes are unspecified. Comparing objects with padding using 'memcmp' may lead to unexpected result. tlosertree.c 127

В 64-битной программе между переменной index и указателем находятся 4 дополнительных байта, предназначенные для того, чтобы выровнять 64-битный указатель по границе 8 байт. Плохая идея сравнивать такие структуры с помощью функции memcmp. В дополнительных байтах для выравнивания лежат произвольные значения. Функция memcmp может счесть структуры неодинаковыми, хотя значения всех полей совпадают.

Ошибки N187-N190. Использование устаревших функций, связанных с криптографией

uint32_t taosSafeRand(void) {
  ....
  if (!CryptGenRandom(hCryptProv, 4, &seed)) return seed;
  ....
}

Предупреждение PVS-Studio: V1109 The 'CryptGenRandom' function is deprecated. Consider switching to an equivalent newer function. osRand.c 56

Другие схожие предупреждения:

V1109 The 'CryptAcquireContextA' function is deprecated. Consider switching to an equivalent newer function. osRand.c 50
V1109 The 'CryptAcquireContextA' function is deprecated. Consider switching to an equivalent newer function. osRand.c 51
V1109 The 'CryptReleaseContext' function is deprecated. Consider switching to an equivalent newer function. osRand.c 58

Заключение

Используйте статический анализ, чтобы:

Повысить качество и надёжность вашего программного обеспечения, а также снизить репутационные риски, связанные с обнаружением уязвимостей нулевого дня в ваших программных продуктах.
Найти ошибки и потенциальные уязвимости ещё на этапе разработки: чем раньше дефект обнаружен, тем дешевле его исправление.
Выявлять паттерны ошибок, о которых ваша команда может даже не подозревать.
Сосредоточиться на обзорах кода (code review) над алгоритмами и высокоуровневыми ошибками, а не всматриваться в имена переменных и расположение скобок.
Писать более простой и надёжный код. Иногда полезны даже ложные срабатывания анализатора. Если код запутал анализатор, то он тем более может запутать человека. Такой код, скорее всего, лучше переписать.
Контролировать качество кода в целом. Например, рост плотности ошибок может сказать, что необходимо уделить больше внимания обучению новых коллег.
Построить процесс разработки безопасного программного обеспечения (РБПО/SSDLC).

Если речь идёт о РБПО по ГОСТ Р 56939-2024, то дополнительно следует уделить внимание выбору инструментов статического анализа. Требования к инструментальным средствам изложены в ГОСТ Р 71207–2024 и включают в себя, например, перечисление типов критических ошибок обязательных для выявления.

Для всех этих задач можно использовать PVS-Studio. Он поддерживает анализ кода на языках C, C++, C#, Java. Работает под управлением операционных систем Windows, Linux, maсOS. Является SAST решением, позволяющим улучшить качество, надёжность и безопасность разрабатываемых проектов.

Инструмент разрабатывается в России с 2008 года (запись в Едином Реестре российского ПО N9837) и содержит более 1000 диагностических правил. Возможно использование в полностью закрытом контуре. Совместим с ГОСТ Р 71207-2024 (Статический анализ программного обеспечения), а также с нормативными документами и требованиями регуляторов (ФСТЭК).

Дополнительные ссылки

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Why SSDLC needs static analysis: a case study of 190 bugs in TDengine.

Необходимость статического анализа для РБПО на примере 190 багов в TDengine