# Топ 10 ошибок в C\+\+ проектах за 2020 год

За окном зима, год стремится к завершению, а значит, пришло время рассмотреть самые интересные ошибки, обнаруженные анализатором PVS\-Studio в 2020 году\.

![0784_Top_10_CPP_Bugs_2020_ru/image1.png](https://import.viva64.com/docx/blog/0784_Top_10_CPP_Bugs_2020_ru/image1.png)

Стоит отметить, что прошедший год ознаменовался большим количеством новых диагностических правил, срабатывания которых позволили им попасть в данный топ\. Также мы продолжаем улучшать ядро анализатора и добавлять новые сценарии его использования, обо всём этом можно почитать в нашем [блоге](https://pvs-studio.ru/ru/blog/posts/)\. Если вам интересны другие поддерживаемые нашим анализатором языки \(C, C\# и Java\), обратите внимание на статьи моих коллег\. Теперь же перейдём непосредственно к самым запомнившимся мне багам, найденным PVS\-Studio за прошедший год\.

## Десятое место: Деление по модулю на единицу

[V1063](https://pvs-studio.ru/ru/docs/warnings/v1063/) The modulo by 1 operation is meaningless\. The result will always be zero\. llvm\-stress\.cpp 631

```cpp
void Act() override {
  ....
  // If the value type is a vector, and we allow vector select,
  // then in 50% of the cases generate a vector select.
  if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 1)) {
    unsigned NumElem =
        cast<FixedVectorType>(Val0->getType())->getNumElements();
    CondTy = FixedVectorType::get(CondTy, NumElem);
  }
  ....
}
```

Разработчик хотел получить случайное значение в диапазоне от 0 до 1, использовав деление по модулю\. Однако операция вида _X%1_ всегда вернёт 0\. В данном случае правильно было бы переписать условие следующим образом: 

```cpp
if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 2))
```

Эта ошибка вошла в топ из статьи: "[Проверка Clang 11 с помощью PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/cpp/0771/)"\.

## Девятое место: Четыре проверки

На следующий участок кода PVS\-Studio выдал четыре предупреждения:

* [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: x \>\= 0\. editor\.cpp 1137
* [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: y \>\= 0\. editor\.cpp 1137
* [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: x < 40\. editor\.cpp 1137
* [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: y < 30\. editor\.cpp 1137

```cpp
int editorclass::at( int x, int y )
{
  if(x<0) return at(0,y);
  if(y<0) return at(x,0);
  if(x>=40) return at(39,y);
  if(y>=30) return at(x,29);

  if(x>=0 && y>=0 && x<40 && y<30)
  {
      return contents[x+(levx*40)+vmult[y+(levy*30)]];
  }
  return 0;
}
```

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

Эта ошибка вошла в топ из статьи: "[VVVVVV??? VVVVVV\!\!\!](https://pvs-studio.ru/ru/blog/posts/cpp/0707/)"\.

## Восьмое место: delete вместо delete\[\]

[V611](https://pvs-studio.ru/ru/docs/warnings/v611/) The memory was allocated using 'new T\[\]' operator but was released using the 'delete' operator\. Consider inspecting this code\. It's probably better to use 'delete \[\] poke\_data;'\. CCDDE\.CPP 410

```cpp
BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type)
{
  ....
  char *poke_data = new char [length + 2*sizeof(int)]; // <=
  ....
  if(DDE_Class->Poke_Server( .... ) == FALSE) {
    CCDebugString("C&C95 - POKE failed!\n");
    DDE_Class->Close_Poke_Connection();
    delete poke_data;                                  // <=
    return (FALSE);
  }

  DDE_Class->Close_Poke_Connection();

  delete poke_data;                                    // <=

  return (TRUE);
}
```

Анализатор обнаружил ошибку, связанную с тем, что память выделена и освобождена несовместимыми между собой способами\. Для освобождения памяти, выделенной под массив, следует использовать оператор _delete\[\]_, а не _delete_\.

Эта ошибка вошла в топ из статьи: "[Код игры Command & Conquer: баги из 90\-х\. Том второй](https://pvs-studio.ru/ru/blog/posts/cpp/0748/)"\.

## Седьмое место: Выход за границу буфера

Рассмотрим функцию _net\_hostname\_get_, которая будет использоваться дальше\.

```cpp
#if defined(CONFIG_NET_HOSTNAME_ENABLE)
const char *net_hostname_get(void);
#else
static inline const char *net_hostname_get(void)
{
  return "zephyr";
}
#endif
```

В данном случае при препроцессировании выбирался вариант, относящийся к ветке _\#else_\. То есть, в препроцессированном файле функция реализуется так:

```cpp
static inline const char *net_hostname_get(void)
{
  return "zephyr";
}
```

Функция возвращает указатель на массив из 7 байт \(учитываем терминальный ноль в конце строки\)\.

Теперь рассмотрим код, приводящий к выходу за границу массива\.

```cpp
static int do_net_init(void)
{
  ....
  (void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);
  ....
}
```

Предупреждение PVS\-Studio: [V512](https://pvs-studio.ru/ru/docs/warnings/v512/) \[CWE\-119\] A call of the 'memcpy' function will lead to the 'net\_hostname\_get\(\)' buffer becoming out of range\. log\_backend\_net\.c 114

После препроцессирования _MAX\_HOSTNAME\_LEN_ раскрывается следующим образом:

```cpp
(void)memcpy(hostname, net_hostname_get(),
    sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx"));
```

Соответственно, при копировании данных возникает выход за границу строкового литерала\. Как это скажется на выполнении программы \- предсказать сложно, так как это приводит к неопределённому поведению\. 

Эта ошибка вошла в топ из статьи: "[Исследуем качество кода операционной системы Zephyr](https://pvs-studio.ru/ru/blog/posts/0721/)"\.

## Шестое место: Что\-то очень странное

```cpp
static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    ((u8_t *)mntpt)[strlen(mntpt)] = '\0';
    memcpy(cpy_mntpt, mntpt, strlen(mntpt));
  }
  return cpy_mntpt;
}
```

Предупреждение PVS\-Studio: [V575](https://pvs-studio.ru/ru/docs/warnings/v575/) \[CWE\-628\] The 'memcpy' function doesn't copy the whole string\. Use 'strcpy / strcpy\_s' function to preserve terminal null\. shell\.c 427

Кто\-то пытался сделать аналог функции _strdup_, но у него это не получилось\.

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

Кажется, что этот терминальный 0 копируется здесь:

```cpp
((u8_t *)mntpt)[strlen(mntpt)] = '\0';
```

Но нет\! Здесь опечатка, из\-за которой терминальный ноль копируется сам в себя\! Обратите внимание, что запись происходит в массив _mntpt_, а не в _cpy\_mntpt_\. В итоге функция _mntpt\_prepare_ возвращает строку, незавершенную терминальным нулём\.

На самом деле программист хотел написать так:

```cpp
((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';
```

Однако всё равно не понятно, зачем сделано так сложно\! Этот код можно упростить до следующего варианта:

```cpp
static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    strcpy(cpy_mntpt, mntpt);
  }
  return cpy_mntpt;
}
```

Эта ошибка вошла в топ из вышеупомянутой статьи: "[Исследуем качество кода операционной системы Zephyr](https://pvs-studio.ru/ru/blog/posts/0721/)"\.

## Пятое место: Неправильная защита от переполнения

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) \[CWE\-570\] Expression 'rel\_wait < 0' is always false\. Unsigned type value is never < 0\. os\_thread\_windows\.c 359

```cpp
static DWORD
get_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;
}
```

В данном случае переменная _rel\_wait_ имеет беззнаковый тип _DWORD_\. А значит, сравнение _rel\_wait < 0_ не имеет смысла, так как результатом всегда является ложь\.

Сама по себе ошибка не очень интересная\. Зато интересно вышло с тем, как её попробовали исправить\. Получилось, что изменения не исправили, а лишь упростили код\. Подробнее про эту историю можно прочитать в статье моего коллеги: "[Почему PVS\-Studio не предлагает автоматические правки кода](https://pvs-studio.ru/ru/blog/posts/cpp/0776/)"\.

Ошибка же вошла в топ из статьи: "[Статический анализ кода коллекции библиотек PMDK от Intel и ошибки, которые не ошибки](https://pvs-studio.ru/ru/blog/posts/cpp/0756/)"\.

## Четвёртое место: Не пиши в std, брат

[V1061](https://pvs-studio.ru/ru/docs/warnings/v1061/) Extending the 'std' namespace may result in undefined behavior\. sized\_iterator\.hh 210

```cpp
// Dirty hack because g++ 4.6 at least wants
// to do a bunch of copy operations.
namespace std {
inline void iter_swap(util::SizedIterator first,
                      util::SizedIterator second)
{
  util::swap(*first, *second);
}
} // namespace std
```

В статье, из которой взято срабатывание: "[Анализ кода проекта DeepSpeech или почему не стоит писать в namespace std](https://pvs-studio.ru/ru/blog/posts/cpp/0768/)" подробно описано, почему не стоит поступать подобным образом\.

## Третье место: Скроллбар, который не смог

[**V501**](https://pvs-studio.ru/ru/docs/warnings/v501/)\. There are identical sub\-expressions to the left and to the right of the '\-' operator: bufferHeight \- bufferHeight TermControl\.cpp 592

```cpp
bool TermControl::_InitializeTerminal()
{
  ....
  auto bottom = _terminal->GetViewport().BottomExclusive();
  auto bufferHeight = bottom;

  ScrollBar().Maximum(bufferHeight - bufferHeight);
  ScrollBar().Minimum(0);
  ScrollBar().Value(0);
  ScrollBar().ViewportSize(bufferHeight);
  ....
}
```

Это, что называется, "срабатывание с историей"\. В данном случае из\-за ошибки не работал скроллбар в Windows Terminal\. По мотивам данного бага написана целая статья, в которой мой коллега провёл исследование и разобрался почему так случилось\. Заинтересовались? Вот она: "[Скроллбар, который не смог](https://pvs-studio.ru/ru/blog/posts/cpp/0718/)"\.

## Второе место: перепутали радиус и высоту

И опять речь пойдёт о нескольких предупреждениях анализатора:

* [V764](https://pvs-studio.ru/ru/docs/warnings/v764/) Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'\. StandardJoints\.cpp 791
* [V764](https://pvs-studio.ru/ru/docs/warnings/v764/) Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'\. StandardJoints\.cpp 833
* [V764](https://pvs-studio.ru/ru/docs/warnings/v764/) Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'\. StandardJoints\.cpp 884

Привожу вызовы функции:

```cpp
NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);
```

А так выглядит её объявление:

```cpp
static NewtonBody* CreateWheel (DemoEntityManager* const scene,
  const dVector& location, dFloat radius, dFloat height)
```

При вызовах функций аргументы были перепутаны местами\.

Эта ошибка вошла в топ из статьи: "[Повторная проверка Newton Game Dynamics статическим анализатором PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/cpp/0729/)"\.

## Первое место: Затирание результата

[V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'color\_name' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 621, 627\. string\.cpp 627

```cpp
static bool parseNamedColorString(const std::string &value,
                                  video::SColor &color)
{
  std::string color_name;
  std::string alpha_string;

  size_t alpha_pos = value.find('#');
  if (alpha_pos != std::string::npos) {
    color_name = value.substr(0, alpha_pos);
    alpha_string = value.substr(alpha_pos + 1);
  } else {
    color_name = value;
  }

  color_name = lowercase(value); // <=

  std::map<const std::string, unsigned>::const_iterator it;
  it = named_colors.colors.find(color_name);
  if (it == named_colors.colors.end())
    return false;
  ....
}
```

Данная функция должна производить разбор названия цвета с параметром прозрачности и вернуть его шестнадцатеричный код\. В зависимости от результата проверки условия в переменную _color\_name_ передается либо результат разбиения строки, либо копия аргумента функции\.

Однако затем в функции _lowercase\(\)_ в нижний регистр переводится не сама полученная строка, а исходный аргумент функции\. В результате мы просто потеряем цвет, который должна была вернуть _parseNamedColorString\(\)_\.

```cpp
color_name = lowercase(color_name);
```

Эта ошибка вошла в топ из статьи: "[PVS\-Studio: Анализ pull request\-ов в Azure DevOps при помощи self\-hosted агентов](https://pvs-studio.ru/ru/blog/posts/cpp/0751/)"\.

## Заключение

За прошедший год мы нашли много ошибок в open source проектах\. Это были привычные ошибки copy\-paste, ошибки в константах, утечки памяти и множество других проблем\. Наш анализатор не стоит на месте и в топе присутствует несколько срабатываний новых диагностик, написанных в этом году\.

Надеюсь, вам понравились собранные ошибки\. Лично мне они показались достаточно интересными\. Но, конечно, ваше видение может отличаться от моего, поэтому вы можете составить свой "Tоп 10\.\.\.", почитав статьи из нашего [блога](https://pvs-studio.ru/ru/blog/posts/) или посмотрев список ошибок, найденных PVS\-Studio в open source проектах\.

Также предлагаю вашему вниманию статьи с топ 10 C\+\+ ошибок прошлых лет: [2016](https://pvs-studio.ru/ru/blog/posts/cpp/0483/), [2017](https://pvs-studio.ru/ru/blog/posts/cpp/0565/), [2018](https://pvs-studio.ru/ru/blog/posts/cpp/0619/), [2019](https://pvs-studio.ru/ru/blog/posts/cpp/0700/)\.