Our website uses cookies to enhance your browsing experience.
New Year is coming! It means, according to tradition, it's time to recall 10 of the most interesting warnings that PVS-Studio found during 2022.
It is worth noting that this year there were not as many articles about project checks as in past years. Articles on our blog have become more varied, and translations of articles by guest authors have also appeared. However, I, as one of our blog's readers (and as the author of some articles from it), still made the top list of bugs found by the PVS-Studio analyzer that are described in our articles on project checks. Let me point out that this top is rather subjective — it has many bugs from articles written by me :).
If you, dear readers, have your own vision of how this top should look, please, share it in the comments. So, let's get started, enjoy reading!
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());
}We see a classic typo in this code fragment:
(SrcTy.isPointer() && DstTy.isScalar()) ||
(DstTy.isScalar() && SrcTy.isPointer())Here is the error:
(SrcTy.isPointer() && DstTy.isScalar()) ||
(DstTy.isScalar() && SrcTy.isPointer())A developer simultaneously changed Src to Dst and Pointer to Scalar in the second line. As a result, it turns out that the same thing is checked twice! The code above is equivalent to:
(SrcTy.isPointer() && DstTy.isScalar()) ||
(SrcTy.isPointer() && DstTy.isScalar())The correct option:
(SrcTy.isPointer() && DstTy.isScalar()) ||
(SrcTy.isScalar() && DstTy.isPointer())This error entered the top of the article: "Examples of errors that PVS-Studio found in 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;
}
}The j index that gives us access to the elements of the keyFrameTimes container is incremented to a value equal to the container size. We can fix it:
while (j < keyFrameTimes.size())
{
// ....
}I took this error from the article: "Checking the Ogre3D framework with the PVS-Studio static analyzer."
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();
}I remember that the first time I saw this piece of code, I couldn't help but facepalm. Let's figure out what's going on here. We see an explicit check of the document pointer for nullptr and its dereference in both branches of the ternary operator.
The code is correct only if the developer aimed to crash the program.
I took this error from the article: "MuditaOS: Will your alarm clock go off? Part I".
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;
// ....
}The analyzer warns that an overflow of the dynamicStates array may occur. There are 4 checks in this code fragment:
Each of these checks is a check of one of the independent flags. For example, the check of if (state.useDynamicDepthBias()):
bool useDynamicDepthBias() const
{
return rs.depthBiasEnable();
}
VkBool32 depthBiasEnable() const
{
return VkBool32(m_depthBiasEnable);
}It turns out that all these 4 checks can be true at the same time. Then 7 lines of the 'dynamicStates[dynamicStateCount++] =....' kind will be executed. On the seventh such line, there will be a call to dynamicStates[6]. It's an array index out of bounds.
This bug is from the top of the article: "Checking the GPCS4 emulator: will we ever be able to play "Bloodborne" on 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());
}Here the analyzer detected that a function, marked as noexcept, calls a function that throws an exception. If an exception arises from the nothrow function's body, the nothrow function calls std::terminate, and the program crashes.
It could make sense to wrap the setName function in the function-try-block and process the exceptional situation there — or one could use something else instead of generating the exception.
I took this error from the article: "MuditaOS: Will your alarm clock go off? Part I".
PVS-Studio issued two warnings at once for the code snippet presented below:
bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
....
const unsigned char* heightfieldData = 0;
....
heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
....
delete heightfieldData;
return ....;
}Let's start with the V773 warning. The memory for the worldImporter pointer was allocated using the new operator and was not released upon exiting the function.
Let's move on to the V611warning and the heightfieldData buffer. Here, the developer cleared the dynamically allocated memory, but did so in the wrong way. Instead of calling delete[] to release previously allocated memory with the new[] operator, they called a simple delete. According to the standard, such code will lead to undefined behavior — here is a link to the corresponding item.
By the way, you can read about why arrays need to be deleted by delete[], and about how it all works in general in article written by my colleague.
Here is the fixed version:
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 ....;
}Also, it's possible to avoid problems with manual memory cleanup by writing more modern code. For example, automatically free up memory by using std::unique_ptr. The code is shorter and more reliable. It will protect against unreleased memory errors in case of early exit from the function:
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 ....;
}You can find this error in the article: "In the world of anthropomorphic animals: PVS-Studio checked 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;
....
}Let's figure out why the analyzer decided that the right part of the condition is always false. For convenience, let's remove all superfluous details and replace the names:
A = ....;
B = ....;
if (A == B) return;
C = ....;
if (A != C || C == B)Let's look at how this code works:
This error entered the top of the article: "Examples of errors that PVS-Studio found in LLVM 15.0."
A couple of analyzer warnings:
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 is an std::optional entity from the pointer to AudioMux::Input. The nested if statement contains the value member function call. The function is guaranteed to return the pointer and will not throw an exception. After, the result is dereferenced.
However, the function may return either a valid — or a null pointer. The plan for the nested if statement was probably to check this pointer. Hm, I also like wrapping pointers and boolean values in std::optional! And then going through the same grief each time :).
Let's see how we can fix it:
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);
}
}
....
}I took this error from the article: "MuditaOS: Will your alarm clock go off? Part I".
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;
}The GetProtectFlag function converts a flag with file access permission from one format to another. However, the function does this incorrectly. The developer did not take into account that the value of VMPF_NOACCESS is zero. Because of this, the if (nOldFlag & VMPF_NOACCESS) condition is always false and the function will never return the PAGE_NOACCESS value.
In addition, the GetProtectFlag function incorrectly converts not only the VMPF_NOACCESS flag, but also other flags. For example, the VMPF_CPU_EXEC flag will be converted to the PAGE_EXECUTE_READWRITE flag.
When a developer who wrote the article was thinking how to fix that issue, his first thought was to write something like this:
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;
}However, in this case, this approach does not work. The thing is, PAGE_NOACCESS, PAGE_READONLY, and other flags are Windows flags and they have their own specifics. For example, there is no PAGE_WRITE flag among them. It is assumed that if there are write permissions, then at least there are also read permissions. For the same reasons, there is no PAGE_EXECUTE_WRITE flag.
In addition, the bitwise "OR" of two Windows flags does not result in a flag corresponding to the sum of the permissions: PAGE_READONLY | PAGE_EXECUTE != PAGE_EXECUTE_READ. Therefore, you need to iterate through all possible flag combinations:
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;
}
}This bug is from the article: "Checking the GPCS4 emulator: will we ever be able to play "Bloodborne" on PC?."
V530 [CWE-252]: The return value of function 'clamp' is required to be utilized. BLI_math_vector.hh 88
So, once upon a time there was code that processed a vector of values. It prevented values from going beyond a certain range.
#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;
}Everything was good. And then a developer decided to abandon the custom CLAMP macro and use the standard std::clamp function. And the commit that supposed to make the code better looked like this:
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;
}It seems like the developer was in a hurry. Do you see the error? Maybe yes, maybe no. Anyway, the developer who wrote the code, didn't notice that it was broken.
The point being — the std::clamp function doesn't change the value of the element in the container:
template <class T>
constexpr const T&
clamp( const T& v, const T& lo, const T& hi );The CLAMP macro used to change the value, but the standard function did not. Now the code is broken and is waiting for someone to notice an error and look for its cause.
Let me point out that we regularly check several open-source projects. Sometimes it allows you to write quick articles and show interesting bugs in code just written.
Although we already written many articles on this topic, I described the warning that seemed the most striking for me. By the way, if you haven't read these articles, then I strongly recommend you do!
As the author of this compilation, I want to give the first place not only to this error, but to the whole series of articles on the topic. In fact, this is a great work of one well-known person (Andrey, hi!) who started a separate genre of articles on our blog that delighted us throughout the year 2022.
The same error entered the top of the article: "How PVS-Studio prevents rash code changes, example N4".
This year was not as prolific in terms of articles on C++ project checks as the past ones. However, I hope that we were still able to delight you with a selection of interesting bugs. We have also written many articles in other genres this year. You can find them on our blog.
It has become a tradition to post such articles on New Year's Eve, so here are the articles with the top 10 bugs found in C and C++ projects of the past years: 2016, 2017, 2018, 2019, 2020, 2021.
0
0
0
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
0