Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл 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.
