Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл 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()):
bool useDynamicDepthBias() const
{
return rs.depthBiasEnable();
}
VkBool32 depthBiasEnable() const
{
return VkBool32(m_depthBiasEnable);
}Получается, все эти 4 проверки могут быть истинными одновременно. Тогда выполнится 7 строк вида 'dynamicStates[dynamicStateCount++] = ....'. На седьмой такой строке произойдёт обращение к dynamicStates[6]. Это выход за границу массива.
Эта ошибка вошла в топ из статьи: "Проверяем эмулятор GPCS4, или сможем ли когда-нибудь поиграть в "Bloodborne" на PC".
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 выдал сразу два предупреждения:
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".
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)Разберём как работает этот код:
Эта ошибка вошла в топ из статьи: "Примеры ошибок, которые может обнаружить PVS-Studio в коде LLVM 15.0".
Пара срабатываний анализатора:
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".
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.