Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you haven't received our response, please do the following:
check your Spam/Junk folder and click the
"Not Spam" button for our message.
This way, you won't miss messages from our team in the future.
Best Warnings — the analyzer's mode that displays 10 most interesting warnings in the output window. We invite you to take a look at the updated Best Warnings mode on the example of the RPCS3 project check.
Best Warnings is a special mechanism for the first acquaintance with the PVS-Studio static analyzer. The full analyzer's log can contain thousands of warnings. Therefore, if you want to evaluate the work of the analyzer and not waste time and effort on viewing a large report that was issued by a not yet configured analyzer, use the Best Warnings mechanism. Open the analyzer's log in the PVS-Studio plugin for Visual Studio and enable Best Warnings:
When you use Best Warnings, the analyzer will display top 10 warnings. This is how the output window looks like after enabling Best Warnings:
This mode was released a year ago. And not so long ago we realised that hardly anyone was using it. Why? Because it was a challenge to find this mode. So we moved the button to the main bar of the panel and gave it a bright icon. In addition, the mode is now available in JetBrains Rider, CLion, IntelliJ IDEA, as well as in C and C++ Compiler Monitoring UI.
The second problem was Best Warnings in the PVS-Studio output window that were not interesting. This is partly due to the fact that we ourselves forgot about our own feature and did not add weights for new diagnostic rules☹ It turned out that 15 new diagnostic rules simply did not appear in Best Warnings mode. Of course, we fixed it. We also looked at what warnings the Best Warnings mode displays for various open-source projects, and changed the old weights of diagnostic rules to make the warnings more interesting.
The third problem was "duplicates". It is not much fun to see two or more warnings of the same diagnostic rule in Best Warnings. There was already a mechanism for removing duplicates — the weights of the second and subsequent warnings decreased proportionally. We have increased the penalty for duplicates, and now they appear less frequently.
You can read more about Best Warnings here.
RPCS3 is a PS3 console emulator. We have checked this project before. The code base of the project is 300,000 lines of C++ code, which is enough to demonstrate the power of Best Warnings. To check the project, I fixed it on the e98b07d commit.
Let's move on to the errors.
V1064 The 'nsec' operand of the modulo operation is less than the '1000000000ull' operand. The result is always equal to the left operand. Thread.cpp 2359
void thread_ctrl::wait_for(u64 usec, [[maybe_unused]] bool alert /* true */)
{
// ....
if (!alert && usec > 0 && usec <= 1000 && fd_timer != -1)
{
struct itimerspec timeout;
u64 missed;
u64 nsec = usec * 1000ull;
timeout.it_value.tv_nsec = (nsec % 1000000000ull);
timeout.it_value.tv_sec = nsec / 1000000000ull;
timeout.it_interval.tv_sec = 0;
timeout.it_interval.tv_nsec = 0;
timerfd_settime(fd_timer, 0, &timeout, NULL);
// ....
}
}Here, the data flow analysis was able to identify an interesting anomaly. In this code snippet, the following happens:
From the first two points, we get that the value of the nsec variable does not exceed 1'000'000, which is less than 1'000'000'000. It turns out that the nsec / 1'000'000'000 expression is always equal to 0, and the nsec %1'000'000'000 expression is equal to nsec.
Next, the calculated values are used to set the timer when the timerfd_settime function is called. The number of milliseconds to wait is passed to the wait_for function. The same value is written to the nsec variable, only in nanoseconds, so multiply by 1'000. The operations of taking the remainder and dividing by 1'000'000'000 are the selection of the fractional part and the whole number of seconds, respectively. Most likely, the wait_for function should receive values not exceeding 1'000 milliseconds (i.e. less than a second), this is indicated by the usec <= 1'000 check. Therefore, it is pointless to select whole and fractional parts — these are unnecessary calculations.
Fixed code:
void thread_ctrl::wait_for(u64 usec, [[maybe_unused]] bool alert /* true */)
{
// ....
if (!alert && usec > 0 && usec <= 1000 && fd_timer != -1)
{
struct itimerspec timeout;
u64 missed;
u64 nsec = usec * 1000ull;
timeout.it_value.tv_nsec = nsec;
timeout.it_value.tv_sec = 0;
timeout.it_interval.tv_sec = 0;
timeout.it_interval.tv_nsec = 0;
timerfd_settime(fd_timer, 0, &timeout, NULL);
// ....
}
}V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 385, 387. lv2_socket_p2ps.cpp 385
bool lv2_socket_p2ps::handle_listening(p2ps_encapsulated_tcp* tcp_header,
[[maybe_unused]] u8* data,
::sockaddr_storage* op_addr)
{
// ....
if ( tcp_header->flags == p2ps_tcp_flags::SYN
&& backlog.size() < max_backlog)
{
if (backlog.size() >= max_backlog)
{
// ....
}
}
// ....
}The analyzer detected two contradictory conditions nested within the another. Indeed, the backlog.size() < max_backlog and backlog.size() >= max_backlog conditions cannot both be met simultaneously. Therefore, the expression of the nested condition will always be false. Most likely, this error occurred as a result of refactoring.
V595 The 'm_finfo' pointer was utilized before it was verified against nullptr. Check lines: 5316, 5344. SPURecompiler.cpp 5316
class spu_llvm_recompiler : public spu_recompiler_base
, public cpu_translator
{
// ....
function_info* m_finfo;
// ....
virtual spu_function_t compile(spu_program&& _func) override
{
// ....
const u32 src = m_finfo->fn ? bb.reg_origin_abs[i]
: bb.reg_origin[i];
// ....
value = m_finfo && m_finfo->load[i] ? m_finfo->load[i]
: m_ir->CreateLoad(regptr);
// ....
}
}In this code fragment, the m_info pointer is first dereferenced, and is checked for nullptr equality down the code. In other places, the m_info pointer is checked before dereference. Most likely, developers forgot the check here. Here's the fixed code:
class spu_llvm_recompiler : public spu_recompiler_base
, public cpu_translator
{
// ....
function_info* m_finfo;
// ....
virtual spu_function_t compile(spu_program&& _func) override
{
// ....
const u32 src = m_info && m_finfo->fn ? bb.reg_origin_abs[i]
: bb.reg_origin[i];
// ....
value = m_finfo && m_finfo->load[i] ? m_finfo->load[i]
: m_ir->CreateLoad(regptr);
// ....
}
}V766 An item with the same key '0x120' has already been added. evdev_joystick_handler.h 135
class evdev_joystick_handler final : public PadHandlerBase
{
const std::unordered_map<u32, std::string> button_list =
{
// ....
{ 0x11d , "0x11d" },
{ 0x11e , "0x11e" },
{ 0x11f , "0x11f" },
{ BTN_JOYSTICK , "Joystick" },
{ BTN_TRIGGER , "Trigger" },
{ BTN_THUMB , "Thumb" },
{ BTN_THUMB2 , "Thumb 2" },
{ BTN_TOP , "Top" },
{ BTN_TOP2 , "Top 2" },
{ BTN_PINKIE , "Pinkie" },
// ....
}
}At first glance at code, it is not clear which two keys are the same. It turns out that BTN_JOYSTICK and BTN_TRIGGER are Linux API constants that equals 0x120. BTN_JOYSTICK is the starting value of the joystick signal constant group and BTN_TRIGGER is the very first constant in the list of that group. Therefore, their values are the same.
The funny thing is that developers have dealt with this issue before and some of the values have been commented out. Just a couple of lines above in the same hash table, you can see the following:
// ....
// { BTN_MOUSE , "Mouse" }, same as BTN_LEFT
{ BTN_LEFT , "Left" },
// ....V597 The compiler could delete the 'memset' function call, which is used to flush 'tmpbuf' buffer. The memset_s() function should be used to erase the private data. sha1.cpp 371
/*
* SHA-1 HMAC final digest
*/
void sha1_hmac_finish( sha1_context *ctx, unsigned char output[20] )
{
unsigned char tmpbuf[20];
sha1_finish( ctx, tmpbuf );
sha1_starts( ctx );
sha1_update( ctx, ctx->opad, 64 );
sha1_update( ctx, tmpbuf, 20 );
sha1_finish( ctx, output );
memset( tmpbuf, 0, sizeof( tmpbuf ) );
}Golden classic. In this fragment, the compiler will delete the memset function call as a result of optimizations, and the data will remain in memory. It is difficult to say whether an attacker will be able to take advantage of this potential vulnerability. However, just in case, it is worth using the memset_s function - its call cannot be deleted by the compiler. There are other ways to achieve a safe memory cleanup.
So, we have reviewed several warnings in the Best Warnings mode. What conclusions can we draw from this?
Firstly, even developers of cool projects such as RPCS3 can make mistakes. The reasons may be different: refactoring, copy paste, poor attention.
Secondly, Best Warnings turned out to be a really convenient mode. The complete log of the RPCS3 project's check contained more than 700 warnings of the High and Medium levels. I would have to spend several hours reviewing all the warnings. In the Best Warnings mode, I looked through only 10 warnings in about half an hour — and I've already got material for the article!
Thirdly, if you want to try the PVS-Studio analyzer, we invite you to download it, request a trial key, and try Best Warnings on your project. If you like the analyzer and want to implement it for regular checks, I recommend checking out this article. Be sure to tell us about your experience! Clean code to you!
0
0
0
0