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

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

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

29.12.2022 14:47

Stolyarrr 1 1900 Источник

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

1021_Top_10_CPP_Bugs_2022_ru/image1.png

Стоит отметить, что в этом году было не так много статей про проверку проектов, как в прошлых. Статьи в нашем блоге стали разнообразней, а также появились переводы статей сторонних авторов. Однако, у меня, как у одного из читателей нашего блога (а также автора некоторых статей из него), всё равно сформировался топ ошибок, найденных анализатором 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 защищает от поспешных правок кода, пример N4".

Заключение

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

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

Комментарии (1)

durnoy
30.12.2022 13:43
#25062894
Шестое место интересное. Исключение в operator= случится, только если Device справа содержит слишком длинное имя. Это значит, что этот девайс ещё раньше как-то создали в обход ограничений setName. Упасть по terminate может быть вполне разумным вариантом.

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

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

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

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

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

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

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

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

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

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

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

Заключение

Комментарии (1)

durnoy