Топ-10 ошибок в C++ проектах за 2022 год

• Десятое место: Классическая опечатка
• Девятое место: Куда уж без выхода за границу контейнера
• Восьмое место: Явный пример разыменования нулевого указателя
• Седьмое место: Учимся считать до семи
• Шестое место: Бросок исключения из noexcept функции
• Пятое место: Неправильная работа с динамической памятью
• Четвёртое место: Сила Symbolic execution
• Третье место: ммм, как мы любим std::optional<T*>
• Второе место: Неправильное использование флага
• Первое место: PVS-Studio защищает от поспешных правок кода!
• Заключение

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

Стоит отметить, что в этом году было не так много статей про проверку проектов, как в прошлых. Статьи в нашем блоге стали разнообразней, а также появились переводы статей сторонних авторов. Однако, у меня, как у одного из читателей нашего блога (а также автора некоторых статей из него), всё равно сформировался топ ошибок, найденных анализатором PVS-Studio и описанных в наших статьях про проверку проектов. Отмечу, что этот топ довольно субъективный — в нём много ошибок из статей, которые писал я :).

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

Десятое место: Классическая опечатка

V501 [CWE-570] There are identical sub-expressions '(SrcTy.isPointer() && DstTy.isScalar())' to the left and to the right of the '||' operator. CallLowering.cpp 1198

static bool isCopyCompatibleType(LLT SrcTy, LLT DstTy) 
{
  if (SrcTy == DstTy)
    return true;

  if (SrcTy.getSizeInBits() != DstTy.getSizeInBits())
    return false;

  SrcTy = SrcTy.getScalarType();
  DstTy = DstTy.getScalarType();

  return (SrcTy.isPointer() && DstTy.isScalar()) ||
         (DstTy.isScalar() && SrcTy.isPointer());
}

В данном фрагменте кода допущена классическая опечатка:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(DstTy.isScalar()  && SrcTy.isPointer())

Ошибка здесь:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(DstTy.isScalar()  && SrcTy.isPointer())

Программист одновременно поменял во второй строчке Src на Dst и Pointer на Scalar. В результате получается, что два раза проверяется одно и то же! Код выше эквивалентен:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(SrcTy.isPointer() && DstTy.isScalar())

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

(SrcTy.isPointer() && DstTy.isScalar()) ||
(SrcTy.isScalar()  && DstTy.isPointer())

Эта ошибка вошла в топ из статьи: "Примеры ошибок, которые может обнаружить PVS-Studio в коде LLVM 15.0".

Девятое место: Куда уж без выхода за границу контейнера

V557 Array overrun is possible. The 'j' index is pointing beyond array bound. OgreAnimationTrack.cpp 219

void AnimationTrack::_buildKeyFrameIndexMap(
  const std::vector<Real>& keyFrameTimes)
{

  // ....

  size_t i = 0, j = 0;
  while (j <= keyFrameTimes.size())                    // <=
  {
    mKeyFrameIndexMap[j] = static_cast<ushort>(i);
    while (i < mKeyFrames.size()
      && mKeyFrames[i]->getTime() <= keyFrameTimes[j]) // <=
      ++i;
    ++j;
  }
}

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

while (j < keyFrameTimes.size())
{
  // ....
}

Эта ошибка вошла в топ из статьи: "Проверка фреймворка Ogre3D статическим анализатором PVS-Studio"

Восьмое место: Явный пример разыменования нулевого указателя

V522 [CERT-EXP34-C] Dereferencing of the null pointer 'document' might take place. TextBlockCursor.cpp 332

auto BlockCursor::begin() -> std::list<TextBlock>::iterator
{
  return document == nullptr 
            ? document->blocks.end() : document->blocks.begin();
}

Помню, что когда я в первый раз увидел данный фрагмент кода, то даже не удержался от фейспалма. Давайте разберёмся, что тут происходит: мы видим явную проверку указателя document на nullptr и его разыменование в обоих ветках тернарного оператора.

Данный код был бы корректен только в том случае, если целью программиста было бы уронить программу.

Эта ошибка вошла в топ из статьи: "MuditaOS: зазвонит ли ваш будильник? Часть 1".

Седьмое место: Учимся считать до семи

V557 [CWE-787] Array overrun is possible. The 'dynamicStateCount ++' index is pointing beyond array bound. VltGraphics.cpp 157

VkPipeline VltGraphicsPipeline::createPipeline(/* .... */) const
{
  // ....
  std::array<VkDynamicState, 6> dynamicStates;
  uint32_t                      dynamicStateCount = 0;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_VIEWPORT;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_SCISSOR;
  if (state.useDynamicDepthBias())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BIAS;
  if (state.useDynamicDepthBounds())
  {
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BOUNDS;
    dynamicStates[dynamicStateCount++] =
                             VK_DYNAMIC_STATE_DEPTH_BOUNDS_TEST_ENABLE;
  }
  if (state.useDynamicBlendConstants())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_BLEND_CONSTANTS;
  if (state.useDynamicStencilRef())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_STENCIL_REFERENCE;
  // ....
}

Анализатор предупреждает, что может произойти переполнение массива dynamicStates. В данном фрагменте кода есть 4 проверки:

• if (state.useDynamicDepthBias())
• if (state.useDynamicDepthBounds())
• if (state.useDynamicBlendConstants())
• if (state.useDynamicStencilRef())

Каждая из них – это проверка одного из независимых флагов. Например, проверка if (state.useDynamicDepthBias()):

bool useDynamicDepthBias() const
{
  return rs.depthBiasEnable();
}

VkBool32 depthBiasEnable() const
{
  return VkBool32(m_depthBiasEnable);
}

Получается, все эти 4 проверки могут быть истинными одновременно. Тогда выполнится 7 строк вида 'dynamicStates[dynamicStateCount++] = ....'. На седьмой такой строке произойдёт обращение к dynamicStates[6]. Это выход за границу массива.

Эта ошибка вошла в топ из статьи: "Проверяем эмулятор GPCS4, или сможем ли когда-нибудь поиграть в "Bloodborne" на PC".

Шестое место: Бросок исключения из noexcept функции

V509 [CERT-DCL57-CPP] The noexcept function '=' calls function 'setName' which can potentially throw an exception. Consider wrapping it in a try..catch block. Device.cpp 48

struct Device
{
  static constexpr auto NameBufferSize = 240;
  ....
  void setName(const std::string &name)
  {
    if (name.size() > NameBufferSize) 
    {
        throw std::runtime_error("Requested name is bigger than buffer 
                                  size");
    }
    strcpy(this->name.data(), name.c_str());
  }
  ....
}

....

Devicei &Devicei::operator=(Devicei &&d) noexcept
{
  setName(d.name.data());
}

Здесь анализатор обнаружил ситуацию, когда из функции, помеченной как noexcept, вызывается функция, бросающая исключение. В случае покидания исключения из тела nothrow-функции вызовется std::terminate, и программа аварийно завершится.

Возможно, разработчикам стоило подумать о том, чтобы обернуть функцию setName в function-try-block и обработать там возникшую исключительную ситуацию или заменить генерацию исключения на что-то ещё.

Эта ошибка вошла в топ из статьи: "MuditaOS: зазвонит ли ваш будильник? Часть 1".

Пятое место: Неправильная работа с динамической памятью

На представленный ниже фрагмент кода PVS-Studio выдал сразу два предупреждения:

• V611 [CERT-MEM51-CPP] 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 [] heightfieldData;'. PhysicsServerCommandProcessor.cpp 4741
• V773 [CERT-MEM31-C, CERT-MEM51-CPP] The function was exited without releasing the 'worldImporter' pointer. A memory leak is possible. PhysicsServerCommandProcessor.cpp 4742

bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
  ....
  const unsigned char* heightfieldData = 0;
  ....
  heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
  ....
  delete heightfieldData;
  return ....;
}

Начнём с предупреждения V773. Память под указатель worldImporter была выделена при помощи оператора new и по выходу из функции не была освобождена.

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

К слову, про то, почему массивы нужно удалять именно через delete[], и про то, как вообще всё это работает, можно прочитать в статье моего коллеги.

Итак, вот исправленный вариант:

bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
  ....
  const unsigned char* heightfieldData = 0;
  ....
  heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
  ....

  delete   worldImporter;
  delete[] heightfieldData;
  return ....;
}

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

bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  auto worldImporter = std::make_unique<btMultiBodyWorldImporter> ();
  ....
  std::unique_ptr<unsigned char[]> heightfieldData;
  ....
  heightfieldData = std::make_unique_for_overwrite<unsigned char[]>
                                (width * height * sizeof(btScalar));
  ....
  return ....;
}

Эта ошибка вошла в топ из статьи: "В мире антропоморфных животных: PVS-Studio проверил Overgrowth".

Четвёртое место: Сила Symbolic execution

V560 [CWE-570] A part of conditional expression is always false: DefaultCC == ToCC. SemaType.cpp 7856

void Sema::adjustMemberFunctionCC(QualType &T, bool IsStatic, bool IsCtorOrDtor,
                                  SourceLocation Loc) {
  ....
  CallingConv CurCC = FT->getCallConv();
  CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic);

  if (CurCC == ToCC)
    return;
  ....
  CallingConv DefaultCC = 
    Context.getDefaultCallingConvention(IsVariadic, IsStatic);

  if (CurCC != DefaultCC || DefaultCC == ToCC)
    return;
  ....
}

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

A = ....;
B = ....;
if (A == B) return;
C = ....;
if (A != C || C == B)

Разберём как работает этот код:

• есть 3 переменные A, B, C, значения которых нам неизвестны;
• но мы знаем, что если A == B, то происходит выход из функции;
• следовательно, если функция продолжает выполняться, то A != B;
• если A != C, то, в силу вычисления по короткой схеме, правое подвыражение не вычисляется;
• если правое подвыражение "C == B" вычисляется, значит A == C;
• если A != B и A == C, то С никак не может быть равно B.

Эта ошибка вошла в топ из статьи: "Примеры ошибок, которые может обнаружить PVS-Studio в коде LLVM 15.0".

Третье место: ммм, как мы любим std::optional<T*>

Пара срабатываний анализатора:

• V571 Recurring check. The 'if (activeInput)' condition was already verified in line 249. ServiceAudio.cpp 250
• V547 Expression 'activeInput' is always true. ServiceAudio.cpp 250

std::optional<AudioMux::Input *> AudioMux::GetActiveInput();

....

auto Audio::handleSetVolume(....) -> std::unique_ptr<AudioResponseMessage>
{
  ....
  if (const auto activeInput = audioMux.GetActiveInput(); activeInput) 
  {
    if (activeInput) 
    {
      retCode = activeInput.value()->audio->SetOutputVolume(clampedValue);
    }
  }
  ....
}

Итак, activeInput — std::optional от указателя на AudioMux::Input. Если посмотреть на код под вложенным if, то видно, что у него вызывается функция-член value. Она гарантированно вернёт нам указатель без броска исключения. После этого результат разыменовывается.

Вот только функция может вернуть как валидный, так и нулевой указатель, который мы, наверное, и хотели проверить во вложенном if. Ммм, я тоже люблю оборачивать указатели и булевые значения в std::optional! И потом наступать на грабли :)

Исправленный вариант кода:

std::optional<AudioMux::Input *> AudioMux::GetActiveInput();

....

auto Audio::handleSetVolume(....) -> std::unique_ptr<AudioResponseMessage>
{
  ....
  if (const auto activeInput = audioMux.GetActiveInput(); activeInput) 
  {
    if (*activeInput) 
    {
      retCode = (*activeInput)->audio->SetOutputVolume(clampedValue);
    }
  }
  ....

}

Эта ошибка вошла в топ из статьи: "MuditaOS: зазвонит ли ваш будильник? Часть 1".

Второе место: Неправильное использование флага

V547 [CWE-570] Expression 'nOldFlag & VMPF_NOACCESS' is always false. PlatMemory.cpp 22

#define PAGE_NOACCESS           0x01
#define PAGE_READONLY           0x02
#define PAGE_READWRITE          0x04
#define PAGE_EXECUTE            0x10
#define PAGE_EXECUTE_READ       0x20
#define PAGE_EXECUTE_READWRITE  0x40

enum VM_PROTECT_FLAG
{
  VMPF_NOACCESS  = 0x00000000,
  VMPF_CPU_READ  = 0x00000001,
  VMPF_CPU_WRITE = 0x00000002,
  VMPF_CPU_EXEC  = 0x00000004,
  VMPF_CPU_RW    = VMPF_CPU_READ | VMPF_CPU_WRITE,
  VMPF_CPU_RWX   = VMPF_CPU_READ | VMPF_CPU_WRITE | VMPF_CPU_EXEC,
};

inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  uint32_t nNewFlag = 0;
  do
  {
    if (nOldFlag & VMPF_NOACCESS)
    {
      nNewFlag = PAGE_NOACCESS;
      break;
    }

    if (nOldFlag & VMPF_CPU_READ)
    {
      nNewFlag = PAGE_READONLY;
    }

    if (nOldFlag & VMPF_CPU_WRITE)
    {
      nNewFlag = PAGE_READWRITE;
    }

    if (nOldFlag & VMPF_CPU_EXEC)
    {
      nNewFlag = PAGE_EXECUTE_READWRITE;
    }

  } while (false);
  return nNewFlag;
}

Функция GetProtectFlag конвертирует флаг с правами доступа к файлу из одного формата в другой. Однако делает это некорректно. Программист не учёл, что значение VMPF_NOACCESS равно нулю. Из-за этого условие if (nOldFlag & VMPF_NOACCESS) всегда ложное, и функция никогда не вернёт значение PAGE_NOACCESS.

Кроме того, функция GetProtectFlag неправильно конвертирует не только флаг VMPF_NOACCESS, но и другие. Например, флаг VMPF_CPU_EXEC будет сконвертирован во флаг PAGE_EXECUTE_READWRITE.

Когда разработчик, писавший статью, думал, как это исправить, первой мыслью было написать что-то в таком роде:

inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  uint32_t nNewFlag = PAGE_NOACCESS;
  if (nOldFlag & VMPF_CPU_READ)
  {
    nNewFlag |= PAGE_READ;
  }

  if (nOldFlag & VMPF_CPU_WRITE)
  {
    nNewFlag |= PAGE_WRITE;
  }

  if (nOldFlag & VMPF_CPU_EXEC)
  {
    nNewFlag |= PAGE_EXECUTE;
  }

  return nNewFlag;
}

Однако в данном случае такой подход не работает. Дело в том, что PAGE_NOACCESS, PAGE_READONLY и остальные – это Windows-флаги со своей спецификой. Например, среди них нет флага PAGE_WRITE. Считается, что если есть права на запись, то, как минимум, есть права еще и на чтение. По тем же причинам нет флага PAGE_EXECUTE_WRITE.

Кроме того, побитовое "ИЛИ" двух Windows-флагов не даёт в результате флаг, соответствующий сумме прав: PAGE_READONLY | PAGE_EXECUTE != PAGE_EXECUTE_READ. Поэтому нужно перебирать все возможные комбинации флагов:

inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  switch (nOldFlag)
  {
    case VMPF_NOACCESS:
      return PAGE_NOACCESS;
    case VMPF_CPU_READ:
      return PAGE_READONLY;
    case VMPF_CPU_WRITE: // same as ReadWrite
    case VMPF_CPU_RW:
      return PAGE_READWRITE;
    case VMPF_CPU_EXEC:
      return PAGE_EXECUTE;
    case VMPF_CPU_READ | VMPF_CPU_EXEC:
      return PAGE_EXECUTE_READ:
    case VMPF_CPU_WRITE | VMPF_CPU_EXEC: // same as ExecuteReadWrite
    case VMPF_CPU_RWX:
      return PAGE_EXECUTE_READWRITE;
    default:
      LOG("unknown PS4 flag");
      return PAGE_NOACCESS;
  }
}

Эта ошибка вошла в топ из статьи: "Проверяем эмулятор GPCS4, или сможем ли когда-нибудь поиграть в "Bloodborne" на PC".

Первое место: PVS-Studio защищает от поспешных правок кода!

V530 [CWE-252]: The return value of function 'clamp' is required to be utilized. BLI_math_vector.hh 88

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

#define CLAMP(a, b, c) \
  { \
    if ((a) < (b)) { \
      (a) = (b); \
    } \
    else if ((a) > (c)) { \
      (a) = (c); \
    } \
  } \
  (void)0

template <typename T> inline T
clamp(const T &a, const bT &min_v, const bT &max_v)
{
  T result = a;
  for (int i = 0; i < T::type_length; i++) {
    CLAMP(result[i], min_v, max_v);
  }
  return result;
}

И было всё хорошо. А потом программист решил, что нет смысла использовать самодельный макрос CLAMP, а лучше воспользоваться стандартной функцией std::clamp. И в коммите, призванном сделать мир лучше, код стал таким:

template <typename T, int Size>
inline vec_base<T, Size>
  clamp(const vec_base<T, Size> &a, const T &min, const T &max)
{
  vec_base<T, Size> result = a;
  for (int i = 0; i < Size; i++) {
    std::clamp(result[i], min, max);
  }
  return result;
}

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

Дело в том, что функция std::clamp не меняет значение элемента в контейнере:

template <class T>
constexpr const T&
clamp( const T& v, const T& lo, const T& hi );

Макрос CLAMP раньше изменял значение, а стандартная функция нет. Поэтому теперь код сломан и ждёт, когда кто-то заметит проявление ошибки и пойдёт искать её причину.

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

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

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

• Как PVS-Studio защищает от поспешных правок кода, пример N6.
• Как PVS-Studio защищает от поспешных правок кода, пример N5.
• Как PVS-Studio защищает от поспешных правок кода, пример N4.
• Как PVS-Studio защищает от поспешных правок кода, пример N3.
• Как PVS-Studio защищает от поспешных правок кода, пример N2.
• Как PVS-Studio защищает от поспешных правок кода.
• 0, 1, 2, Фредди забрал Blender

Эта же ошибка вошла в топ из статьи: "Как PVS-Studio защищает от поспешных правок кода, пример N4".

Заключение

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

Писать такие новогодние статьи уже стало традицией, и поэтому я предлагаю вашему вниманию статьи с топ-10 ошибок в C и C++ проектах прошлых лет: 2016, 2017, 2018, 2019, 2020, 2021.