In the first article about qdEngine, we’ve discussed 10 errors selected by the PVS-Studio plugin. However, we have 10 more bugs that are worth mentioning. As they say, it’s better to learn from other people’s mistakes. The bugs also demonstrate well the capability of PVS-Studio to detect various defect patterns.
在关于 qdEngine 的第一篇文章中，我们讨论了 PVS-Studio 插件选择的 10 个错误。但是，我们还有 10 个值得一提的错误。正如他们所说，最好从别人的错误中吸取教训。这些错误还很好地展示了 PVS-Studio 检测各种缺陷模式的能力。
Make sure to catch up on the previous articles about checking the qdEngine game engine:
请务必了解之前有关检查 qdEngine 游戏引擎的文章：
Top 10 warnings issued by PVS-Studio
PVS-Studio 发出的 10 大警告
Simplifying C++ code 简化 C++ 代码
Without further delay, let’s move on to looking at the collection of bugs 🙂
事不宜迟，让我们继续查看 bug 的集合:)
Warning N1: error in error handler
警告 N1：错误处理程序中的错误
Error handlers are usually not tested. Writing unit tests for them is difficult and uninteresting. In manual testing mode, they’re difficult to access (it’s difficult to create a scenario that will cause the particular error to occur in the application).
错误处理程序通常不经过测试。为他们编写单元测试既困难又无趣。在手动测试模式下，它们很难访问（很难创建会导致应用程序中发生特定错误的方案）。
As a result, error handlers often contain errors. I’ll write a separate article about it sometime.
因此，错误处理程序通常包含错误。我会在某个时候写一篇关于它的单独文章。
This is where static code analysis tools such as PVS-Studio come in. Analyzers check code regardless of how often (with what probability) it’s executed while the application is running. That’s why analyzers can find errors in rarely used code. Let’s look at such a case:
这就是 PVS-Studio 等静态代码分析工具的用武之地。分析器会检查代码，而不管代码在应用程序运行时执行的频率（概率）如何。这就是为什么分析器可以在很少使用的代码中发现错误的原因。让我们看一下这样的案例：
class CAVIGenerator
{
….
_bstr_t m_sFile;
….
};
HRESULT CAVIGenerator::InitEngine()
{
….
TCHAR szBuffer[1024];
….
if (hr != AVIERR_OK)
{
_tprintf(szBuffer,
_T(“AVI Engine failed to initialize. Check filename %s.”),m_sFile);
m_sError=szBuffer;
….
};
The PVS-Studio warning: V510 The ‘printf’ function is not expected to receive class-type variable as third actual argument. AVIGenerator.cpp 132
PVS-Studio 警告：V510 “printf”函数不应接收类类型变量作为第三个实际参数。AVIGenerator.cpp 132
The point is that m_sFile is a class of the _bstr_t type. An attempt to display the class contents as a simple string leads to undefined behavior. In reality, garbage characters are likely to be displayed instead of the file name. If there are too many garbage characters, the szBuffer[1024] buffer overflows. A buffer overflow, in its turn, can be seen as a potential vulnerability.
关键是 m_sFile 是_bstr_t类型的类。尝试将类内容显示为简单字符串会导致未定义的行为。实际上，可能会显示垃圾字符而不是文件名。如果垃圾字符过多，则 szBuffer[1024] 缓冲区将溢出。反过来，缓冲区溢出可以被视为潜在的漏洞。
So, use PVS-Studio to prevent such classic bugs in error handlers.
因此，请使用 PVS-Studio 来防止错误处理程序中出现此类经典 bug。
We can fix the code using overloaded operators of the _bstr_t class:
我们可以使用 _bstr_t 类的重载运算符来修复代码：
// Extract the pointers to the encapsulated Unicode or multibyte BSTR object.
operator wchar_t*
operator char*
The fixed code looks like this:
修复后的代码如下所示：
_tprintf(szBuffer,
_T(“AVI Engine failed to initialize. Check filename %s.”),
(LPCSTR)m_sFile);
Depending on the compilation mode (whether it’s a UNICODE application or not), the LPCSTR type is expanded to const wchar_t* or const char*.
根据编译模式（无论是否是 UNICODE 应用程序），LPCSTR 类型将扩展为 const wchar_t* 或 const char*。
Warning N2: unreachable code
警告 N2：无法访问的代码
bool qdGameScene::follow_path_seek(qdGameObjectMoving* pObj,
bool lock_target)
{
if (qdGameObjectMoving::FOLLOW_UPDATE_PATH == pObj->follow_condition())
selected_object_->set_grid_zone_attributes(sGridCell::CELL_SELECTED);
return pObj->move(selected_object_->last_move_order(), lock_target);
if (qdGameObjectMoving::FOLLOW_UPDATE_PATH == pObj->follow_condition())
selected_object_->drop_grid_zone_attributes(sGridCell::CELL_SELECTED);
}
The PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. qd_game_scene.cpp 1245
PVS-Studio 警告：检测到 V779 无法访问的代码。可能存在错误。qd_game_scene.cpp 1245
The code after the return operator isn’t executed.
返回运算符后面的代码不执行。
The block of unreachable code is exactly the same as the one before the return statement. Most likely, programmers accidentally created the repeated code as a result of careless code editing or branch merging. I think they could just delete the code below return.
不可访问代码块与 return 语句之前的代码块完全相同。最有可能的是，程序员由于粗心的代码编辑或分支合并而意外创建了重复的代码。我认为他们可以删除返回下方的代码。
Warning N3: new [] / delete
警告 N3：新 [] / 删除
The qdEngine engine code has a lot of manual memory management. As a result, the code contains many errors related to memory allocation and deallocation. I’d recommend those who will further develop the engine pay particular attention to the revision of the memory handling code.
qdEngine 引擎代码有很多手动内存管理。因此，该代码包含许多与内存分配和释放相关的错误。我建议那些将进一步开发引擎的人特别注意内存处理代码的修订。
bool grFont::load_index(XStream& fh)
{
int buf_sz = fh.size();
char* buf = new char[buf_sz];
….
delete buf;
return true;
}
The PVS-Studio warning: V611 The memory was allocated using the ‘operator new[]’ but was released using the ‘operator delete’. The ‘delete[] buf;’ statement should be used instead. gr_font.cpp 72
PVS-Studio 警告：V611 内存是使用“operator new[]”分配的，但使用“operator delete”释放。应改用 ‘delete[] buf; 语句。gr_font.cpp 72
An array is allocated, so the operator delete[] should be used to deallocate it. The fixed code:
分配了一个数组，因此应使用运算符 delete[] 来解除分配它。固定代码：
bool grFont::load_index(XStream& fh)
{
int buf_sz = fh.size();
char* buf = new char[buf_sz];
….
delete[] buf;
return true;
}
No, though. Such code is incorrect, too. The right way is to use smart pointers:
不过，不。这样的代码也是不正确的。正确的方法是使用智能指针：
bool grFont::load_index(XStream& fh)
{
int buf_sz = fh.size();
auto buf = std::make_unique<char[]>(buf_sz);
….
return true;
}
P.S.: More code fragments with the same error:
P.S.：更多代码片段有相同的错误：
V611 The memory was allocated using the ‘operator new[]’ but was released using the ‘operator delete’. The ‘delete[] buf;’ statement should be used instead. gr_font.cpp 101
V611 内存使用“运算符 new[]”分配，但使用“运算符 delete”释放。应改用 ‘delete[] buf; 语句。gr_font.cpp 101
V611 The memory was allocated using the ‘operator new[]’ but was released using the ‘operator delete’. The ‘delete[] alpha_buffer_;’ statement should be used instead. Check lines: 25, 168. gr_font.cpp 25
V611 内存使用“运算符 new[]”分配，但使用“运算符 delete”释放。应改用 ‘delete[] alpha_buffer_;’ 语句。检查行：25、168。gr_font.cpp 25
V611 The memory was allocated using the ‘operator new[]’ but was released using the ‘operator delete’. The ‘delete[] buffer;’ statement should be used instead. qda_editor.cpp 1012
V611 内存使用“运算符 new[]”分配，但使用“运算符 delete”释放。应改用 ‘delete[] buffer;’ 语句。qda_editor.cpp 1012
Warning N4: function doesn’t return value
警告 N4：函数不返回值
class qdSound : public qdNamedObject, public qdResource
{
….
//! Returns the sound duration in seconds.
float length() const {
#ifndef __QD_SYSLIB__
return sound_.length();
#endif
}
….
}
The PVS-Studio warning: V591 Non-void function should return a value. qd_sound.h 70
PVS-Studio 警告：V591 Non-void 函数应返回一个值。qd_sound.h 70
If the __QD_SYSLIB__ macro is defined, the length function looks like this:
如果定义了 __QD_SYSLIB__ 宏，则 length 函数如下所示：
float length() const {
}
Calling such a function results in undefined behavior.
调用此类函数会导致未定义的行为。
Note. Some programmers believe that undefined behavior in such a case results in the function returning a random value. Nope. When undefined behavior is invoked, anything can happen. You can see an interesting example in this article.
注意。一些程序员认为，在这种情况下，未定义的行为会导致函数返回随机值。不。当调用未定义的行为时，任何事情都可能发生。您可以在本文中看到一个有趣的示例。
I’d suggest fixing the code like this:
我建议像这样修复代码：
#ifndef __QD_SYSLIB__
float length() const {
return sound_.length();
}
#endif
This causes some of the external code to stop compiling. Well, that’s great. We need to rewrite this code too. It makes no sense as it calls to the “dead” length function.
这会导致某些外部代码停止编译。嗯，那太好了。我们也需要重写这段代码。它没有意义，因为它调用了“死”长度函数。
If such a radical approach isn’t a good option for some reason, we can settle for a compromise:
如果出于某种原因，这种激进的方法不是一个好的选择，我们可以妥协：
#ifndef __QD_SYSLIB__
float length() const {
return sound_.length();
}
#else
[[noreturn]] float length() const {
throw std::logic_error { “Function not implemented” };
}
#endif
Warning N5: memory leak
警告 N5：内存泄漏
template<class FilterClass>
LineContribType* C2PassScale<FilterClass>::AllocContributions(UINT uLineLength,
UINT uWindowSize)
{
static LineContribType line_ct;
LineContribType *res = new LineContribType;
line_ct.WindowSize = uWindowSize;
line_ct.LineLength = uLineLength;
if (contribution_buffer_.size() < uLineLength)
contribution_buffer_.resize(uLineLength);
line_ct.ContribRow = &*contribution_buffer_.begin();
if (weights_buffer_.size() < uLineLength * uWindowSize)
weights_buffer_.resize(uLineLength * uWindowSize);
double* p = &*weights_buffer_.begin();
for (UINT u = 0; u < uLineLength; u++) {
line_ct.ContribRow[u].Weights = p;
p += uWindowSize;
}
return &line_ct;
}
The PVS-Studio warning: V773 The function was exited without releasing the ‘res’ pointer. A memory leak is possible. 2PassScale.h 107
PVS-Studio 警告：V773 该函数已退出，但未释放“res”指针。内存泄漏是可能的。2PassScale.h 107
The following line in the code is redundant:
代码中的以下行是多余的：
LineContribType *res = new LineContribType;
The created object isn’t used or deleted anywhere. The only thing that happens is a memory leak.
创建的对象不会在任何地方使用或删除。唯一发生的是内存泄漏。
Warning N6: error in logic
警告 N6：逻辑错误
bool qdConditionGroup::remove_condition(int condition_id)
{
….
conditions_container_t::iterator it1 = std::find(conditions_.begin(),
conditions_.end(),
condition_id);
if (it1 != conditions_.end())
return false;
conditions_.erase(it1);
return true;
}
The PVS-Studio warning: V783 Dereferencing of the invalid iterator ‘it1’ might take place. qd_condition_group.cpp 58
PVS-Studio 警告：可能会发生 V783 取消引用无效迭代器“it1”的情况。qd_condition_group.cpp 58
An attempt is made to delete a nonexistent element:
尝试删除不存在的元素：
conditions_.erase(conditions_.end());
Most likely, the condition contains a logic error and should look like this:
最有可能的是，该条件包含逻辑错误，应如下所示：
if (it1 == conditions_.end())
return false;
conditions_.erase(it1);
Warning N7: dangerous handling of the HRESULT type
警告 N7：HRESULT 类型的危险处理
class winVideo
{
….
struct IGraphBuilder* graph_builder_;
….
};
bool winVideo::open_file(const char *fname)
{
….
if (graph_builder_ -> RenderFile(wPath,NULL))
{
close_file(); return false;
}
….
}
At first glance, the code looks fine. However, the RenderFile function returns the HRESULT type:
乍一看，代码看起来不错。但是，RenderFile 函数返回 HRESULT 类型：
HRESULT RenderFile(
[in] LPCWSTR lpcwstrFile,
[in] LPCWSTR lpcwstrPlayList
);
So, the check is unsafe.
所以，检查是不安全的。
The PVS-Studio warning: V545 Such conditional expression of ‘if’ statement is incorrect for the HRESULT type value ‘graph_builder_->RenderFile(wPath, 0)’. The SUCCEEDED or FAILED macro should be used instead. WinVideo.cpp 139
PVS-Studio 警告：V545 对于 HRESULT 类型值“graph_builder_->RenderFile（wPath， 0）”，“if”语句的此类条件表达式不正确。应改用 SUCCEEDED 或 FAILED 宏。WinVideo.cpp 139
HRESULT is a 32-bit value divided into three different fields: an error severity code, a facility code, and an error code. Macros such as SUCCEEDED and FAILED are used to check values of the HRESULT type.
HRESULT 是一个 32 位值，分为三个不同的字段：错误严重性代码、设施代码和错误代码。SUCCEEDED 和 FAILED 等宏用于检查 HRESULT 类型的值。
The HRESULT type has many states: 0L (S_OK), 0x80000002L (Ran out of memory), 0x80004005L (unspecified failure), etc. Note that the S_OK state is coded as 0.
HRESULT 类型具有多种状态：0L （S_OK） 、0x80000002L （内存不足） 、0x80004005L （未指定失败） 等。请注意，S_OK状态编码为 0。
So, any status other than S_OK is considered an error and the function terminates. Overall, this code seems to work correctly now but it’s unsafe. A little refactoring, in case someone thinks that the RenderFile function returns the bool type, results in a bug.
因此，除 S_OK 以外的任何状态都被视为错误，并且函数终止。总的来说，这段代码现在似乎可以正常工作，但它是不安全的。如果有人认为 RenderFile 函数返回 bool 类型，则稍作重构会导致错误。
The correct check looks like this:
正确的检查如下所示：
if (FAILED(graph_builder_ -> RenderFile(wPath,NULL)))
{
close_file(); return false;
}
Warning N8: false instead of nullptr
警告 N8：false 而不是 nullptr
This code fragment, like the one we’ve discussed above, works. However, I can’t say it’s correct.
这个代码片段，就像我们上面讨论的那个一样，是有效的。但是，我不能说这是正确的。
const char* qdInterfaceDispatcher::get_save_title() const
{
if (!cur_screen_)
return false;
….
return false;
}
PVS-Studio warnings for the “return false” lines:
“return false”行的 PVS-Studio 警告：
V601 The ‘false’ value is implicitly cast to the pointer. qd_interface_dispatcher.cpp 772
V601 “false”值隐式强制转换为指针。qd_interface_dispatcher.cpp 772
V601 The ‘false’ value is implicitly cast to the pointer. qd_interface_dispatcher.cpp 783
V601 “false”值隐式强制转换为指针。qd_interface_dispatcher.cpp 783
The false value is implicitly converted to a null pointer, so the code works as intended. However, for the sake of beauty and decency, let’s use nullptr:
false 值被隐式转换为 null 指针，因此代码按预期工作。但是，为了美观和体面，让我们使用 nullptr：
const char* qdInterfaceDispatcher::get_save_title() const
{
if (!cur_screen_)
return nullptr;
….
return nullptr;
}
Warning N9: something’s wrong, I can feel it
警告N9：有些不对劲，我能感觉到
I’m sorry for the long code fragment below. I haven’t worked out how to shorten it without making it even more confusing.
对于下面的长代码片段，我深表歉意。我还没有弄清楚如何在不让它更加混乱的情况下缩短它。
Vect2s
qdGameObjectMoving::get_nearest_walkable_point(const Vect2s& target) const
{
….
// Go from end. If we reach passable point different from starting one
bool fir_step = true;
if (abs(x2 – x1) > abs(y2 – y1)) {
int dx = int(float(x2 – x1)/dr.x);
do {
if (false == is_walkable(Vect2s(r.xi(),r.yi())))
{
// If there’s only the first step, then it’s a failure
if (fir_step) return Vect2s(-1, -1);
r -= dr;
return Vect2s(r.xi(),r.yi());
}
fir_step = false;
r += dr;
} while (–dx >= 0);
}
else {
int dy = int(float(y2 – y1)/dr.y);
do {
if (false == is_walkable(Vect2s(r.xi(),r.yi())))
{
if (fir_step) return Vect2s(-1, -1);
r -= dr;
return Vect2s(r.xi(),r.yi());
}
fir_step = false;
r += dr;
} while (–dy >= 0);
}
// If the step was never taken
if (fir_step) return trg;
….
}
Take a look at the loops. If they don’t end with exiting the function, the fir_step variable is always false at the end of the loops.
看看循环。如果它们不以退出函数结束，则fir_step变量在循环末尾始终为 false。
So, the following code doesn’t make any sense:
因此，以下代码没有任何意义：
// If the step was never taken
if (fir_step) return trg;
The PVS-Studio analyzer warns us about that: V547 Expression ‘fir_step’ is always false. qd_game_object_moving.cpp 1724
PVS-Studio 分析器警告我们：V547 表达式“fir_step”始终为 false。qd_game_object_moving.cpp 1724
The check line is redundant, or there’s a logic error in the loops. Unfortunately, I can’t say for sure as I’m not familiar with the project code.
检查行是多余的，或者循环中存在逻辑错误。不幸的是，我不能肯定地说，因为我不熟悉项目代码。
Warning N10: strictly aligned
警告 N10：严格对齐
bool zipContainer::find_index()
{
const int buf_sz = 1024;
char buf[buf_sz];
stream_.seek(0,XS_END);
while (stream_.tell() >= buf_sz) {
stream_.seek(-buf_sz,XS_CUR);
stream_.read(buf,buf_sz);
const unsigned long idx_signature = 0x06054b50L;
for (int i = 0; i < buf_sz – sizeof(long) * 3; i++) {
if (*((unsigned long*)(buf + i)) == idx_signature) {
XBuffer xbuf(buf + i + sizeof(long) + sizeof(unsigned short) * 4,
sizeof(long) * 2);
xbuf > index_size_ > index_offset_;
return true;
}
}
}
return false;
}
The PVS-Studio warning: V1032 The pointer ‘buf’ is cast to a more strictly aligned pointer type. zip_container.cpp 237
PVS-Studio 警告：V1032 指针“buf”被强制转换为更严格对齐的指针类型。zip_container.cpp 237
The 0x06054b50L signature is searched for in the byte buffer. It’s done in an unportable and dangerous way.
在字节缓冲区中搜索 0x06054b50L 签名。它是以一种不可移植和危险的方式完成的。
To begin with, we can’t be sure what signature is being searched for. The thing is that the long type has different sizes on different platforms. In most cases, long can be either 32-bit or 64-bit. Depending on the amount of memory, the following things can be searched for:
首先，我们无法确定正在搜索的签名。问题是长型在不同平台上有不同的大小。在大多数情况下，long 可以是 32 位或 64 位。根据内存量，可以搜索以下内容：
the 4-byte signature — 0x06054b50;
4 字节签名 — 0x06054b50;
or the 8-byte signature — 0x0000000006054b50.
或 8 字节签名 — 0x0000000006054b50。
The signature is hardly expected to have different sizes. It’s most likely always be the 4-byte one. So, the programmer would be more correct to use the uint32_t type of the fixed size.
预计签名几乎不会有不同的大小。它很可能总是 4 字节的。因此，程序员使用固定大小的uint32_t类型会更正确。
However, the analyzer warns us about a different thing. The point is that a pointer of the char* type is used to search for the signature. To perform the check, the pointer address (char *) is interpreted as a block of 4/8 bytes (unsigned long *) at each iteration. Here’s a picture for demonstration:
但是，分析器警告我们另一件事。关键是 char* 类型的指针用于搜索签名。为了执行检查，指针地址 （char *） 在每次迭代时被解释为 4/8 字节（无符号长 *）的块。这是一张演示图片：
The issue is that alignment is not considered. Values of the unsigned long type should be located at addresses that are multiples of 4 or 8 bytes (or some other value, depending on the architecture).
问题是没有考虑对齐。unsigned long 类型的值应位于 4 或 8 字节的倍数（或其他值，具体取决于体系结构）的地址上。
Accessing an unaligned address results in undefined behavior. It’s impossible to predict how it can show itself. It depends on the architecture of the microprocessor and compiler. Here are a few possible scenarios:
访问未对齐的地址会导致未定义的行为。无法预测它如何表现出来。这取决于微处理器和编译器的架构。以下是几种可能的情况：
The program crashes. 程序崩溃。
An incorrect value is read. For example, the two low-order pointer bits may be ignored, and reading is still done within boundaries that are multiples of 4 bytes.
读取的值不正确。例如，可以忽略两个低阶指针位，并且仍然在 4 字节的倍数边界内进行读取。
The reading of data will be slowed down because the processor has to perform additional operations.
数据的读取速度会变慢，因为处理器必须执行其他操作。
Everything works fine, at least for now.
一切正常，至少现在是这样。
We can fix the issue in a number of ways. The easiest way is to use the memcmp function, which works at the byte-array level.
我们可以通过多种方式解决此问题。最简单的方法是使用 memcmp 函数，该函数在字节数组级别工作。
if (memcmp(buf + i, &idx_signature, sizeof(idx_signature)) == 0)
Is it possible to write the code in modern C++ without using the memcmp function from the C world, though?
但是，是否可以在不使用 C 世界的 memcmp 函数的情况下用现代 C++ 编写代码？
Yes, but honestly, the code gets a little longer. So, I bring such an option just for the fun of it:
是的，但老实说，代码会变长一些。所以，我带来这样的选择只是为了好玩：
const uint32_t idx_sig = 0x06054b50L;
const std::ranges::subrange
haystack { buf, buf + buf_sz – sizeof(idx_sig) * 3 };
const auto needle = std::bit_cast<std::array<char, sizeof(idx_sig)>>(idx_sig);
if (auto res = std::ranges::search(haystack, needle); !std::empty(res))
{
XBuffer xbuf(it + sizeof(uint32_t) + sizeof(uint16_t) * 4,
sizeof(uint32_t) * 2);
xbuf > index_size_ > index_offset_;
return true;
}
Is there anything else left?
还有别的吗？
We’ve got another error that’s worth looking into. It’s related to object-oriented programming and requires detailed analysis, so I’ll put it in a separate, last article.
我们还有另一个值得研究的错误。它与面向对象编程有关，需要详细分析，因此我将把它放在单独的最后一篇文章中。
