Top 10 errors in C ++ projects in 2020
Winter is outside the window, the year is striving to end, which means that the time has come to consider the most interesting errors detected by the PVS-Studio analyzer in 2020.
It is worth noting that the past year was marked by a large number of new diagnostic rules, the triggering of which allowed them to get into this top. We also continue to improve the analyzer core and add new scenarios for its use, you can read about all this in our blog… If you are interested in other languages supported by our analyzer (C, C # and Java), take a look at the articles of my colleagues. Now let’s move on to the most memorable bugs found by PVS-Studio over the past year.
Tenth place: Modulo division by one
V1063 The modulo by 1 operation is meaningless. The result will always be zero. llvm-stress.cpp 631
void Act() override {
....
// If the value type is a vector, and we allow vector select,
// then in 50% of the cases generate a vector select.
if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 1)) {
unsigned NumElem =
cast<FixedVectorType>(Val0->getType())->getNumElements();
CondTy = FixedVectorType::get(CondTy, NumElem);
}
....
}The developer wanted to get a random value between 0 and 1 by using modulo division. However, an operation of the form X% 1 will always return 0. In this case, it would be correct to rewrite the condition as follows:
if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 2))This error made it to the top from the article: “Checking Clang 11 with PVS-Studio“.
Ninth place: Four checks
PVS-Studio issued four warnings for the next section of the code:
- V560 A part of conditional expression is always true: x> = 0. editor.cpp 1137
- V560 A part of conditional expression is always true: y> = 0. editor.cpp 1137
- V560 A part of conditional expression is always true: x <40. editor.cpp 1137
- V560 A part of conditional expression is always true: y <30. editor.cpp 1137
int editorclass::at( int x, int y )
{
if(x<0) return at(0,y);
if(y<0) return at(x,0);
if(x>=40) return at(39,y);
if(y>=30) return at(x,29);
if(x>=0 && y>=0 && x<40 && y<30)
{
return contents[x+(levx*40)+vmult[y+(levy*30)]];
}
return 0;
}All warnings refer to the latter if-expression. The problem is that all four checks that are performed in it will always return true… I would not say that this is a serious mistake, but it turned out quite funny. In general, these checks are redundant and can be removed.
This error made it to the top from the article: “VVVVVV ??? VVVVVV !!!“.
Eighth place: delete instead of delete[]
V611 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 [] poke_data; ‘. CCDDE.CPP 410
BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type)
{
....
char *poke_data = new char [length + 2*sizeof(int)]; // <=
....
if(DDE_Class->Poke_Server( .... ) == FALSE) {
CCDebugString("C&C95 - POKE failed!n");
DDE_Class->Close_Poke_Connection();
delete poke_data; // <=
return (FALSE);
}
DDE_Class->Close_Poke_Connection();
delete poke_data; // <=
return (TRUE);
}The analyzer has detected an error related to the fact that memory has been allocated and freed in incompatible ways. To free memory allocated for an array, use the operator delete[], but not delete…
This error made it to the top from the article: “Command & Conquer game code: bugs from the 90s. Volume two“.
Seventh place: Buffer out of bounds
Consider the function net_hostname_getwhich will be used further.
#if defined(CONFIG_NET_HOSTNAME_ENABLE)
const char *net_hostname_get(void);
#else
static inline const char *net_hostname_get(void)
{
return "zephyr";
}
#endifIn this case, during preprocessing, the option related to the branch #else… That is, in the preprocessed file, the function is implemented as follows:
static inline const char *net_hostname_get(void)
{
return "zephyr";
}The function returns a pointer to an array of 7 bytes (take into account the terminal zero at the end of the string).
Now let’s look at the code that leads to the overflow of the array.
static int do_net_init(void)
{
....
(void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);
....
}PVS-Studio warning: V512 [CWE-119] A call of the ‘memcpy’ function will lead to the ‘net_hostname_get ()’ buffer becoming out of range. log_backend_net.c 114
After preprocessing MAX_HOSTNAME_LEN is revealed as follows:
(void)memcpy(hostname, net_hostname_get(),
sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx"));Accordingly, when copying data, an overrun of the string literal occurs. How this will affect program execution is difficult to predict, as it leads to undefined behavior.
This error made it to the top from the article: “Examining the code quality of the Zephyr operating system“.
Sixth place: Something very strange
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
((u8_t *)mntpt)[strlen(mntpt)] = ' ';
memcpy(cpy_mntpt, mntpt, strlen(mntpt));
}
return cpy_mntpt;
}PVS-Studio warning: V575 [CWE-628] The ‘memcpy’ function doesn’t copy the whole string. Use ‘strcpy / strcpy_s’ function to preserve terminal null. shell.c 427
Someone tried to make an analog of the function strdup, but he didn’t succeed.
Let’s start with the analyzer warning. It informs that the function memcpy copies a line, but does not copy terminal zero, and this is very suspicious.
This terminal 0 seems to be copied here:
((u8_t *)mntpt)[strlen(mntpt)] = ' ';But no! Here’s a typo that causes the terminal zero to be copied into itself! Please note that writing is to an array mntpt, not in cpy_mntpt… As a result, the function mntpt_prepare returns a string, not terminated with a terminal null.
In fact, the programmer wanted to write like this:
((u8_t *)cpy_mntpt)[strlen(mntpt)] = ' ';However, it is still not clear why it was done so difficult! This code can be simplified to the following:
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
strcpy(cpy_mntpt, mntpt);
}
return cpy_mntpt;
}This bug made it to the top from the above article: “Examining the code quality of the Zephyr operating system“.
Fifth place: Incorrect overflow protection
V547 [CWE-570] Expression ‘rel_wait <0' is always false. Unsigned type value is never <0. os_thread_windows.c 359
static DWORD
get_rel_wait(const struct timespec *abstime)
{
struct __timeb64 t;
_ftime64_s(&t);
time_t now_ms = t.time * 1000 + t.millitm;
time_t ms = (time_t)(abstime->tv_sec * 1000 +
abstime->tv_nsec / 1000000);
DWORD rel_wait = (DWORD)(ms - now_ms);
return rel_wait < 0 ? 0 : rel_wait;
}In this case, the variable rel_wait is of unsigned type DWORD… So the comparison rel_wait <0 makes no sense, since the result is always true.
The error itself is not very interesting. But it turned out interesting how they tried to fix it. It turned out that the changes were not fixed, but only simplified the code. You can read more about this story in my colleague’s article: “Why PVS-Studio doesn’t offer automatic code edits“.
The error entered the top from the article: “Static Code Analysis of Intel’s PMDK Library Collection and Errors That Are Not Errors“.
Fourth place: Don’t write to std, brother
V1061 Extending the ‘std’ namespace may result in undefined behavior. sized_iterator.hh 210
// Dirty hack because g++ 4.6 at least wants
// to do a bunch of copy operations.
namespace std {
inline void iter_swap(util::SizedIterator first,
util::SizedIterator second)
{
util::swap(*first, *second);
}
} // namespace stdIn the article from which the trigger is taken: “Analysis of the code of the DeepSpeech project or why it is not worth writing in the namespace std“describes in detail why you should not do this.
Third place: Scrollbar, which failed
V501… There are identical sub-expressions to the left and to the right of the ‘-‘ operator: bufferHeight – bufferHeight TermControl.cpp 592
bool TermControl::_InitializeTerminal()
{
....
auto bottom = _terminal->GetViewport().BottomExclusive();
auto bufferHeight = bottom;
ScrollBar().Maximum(bufferHeight - bufferHeight);
ScrollBar().Minimum(0);
ScrollBar().Value(0);
ScrollBar().ViewportSize(bufferHeight);
....
}This is what is called “triggering with history”. In this case, due to an error, the scrollbar in Windows Terminal did not work. A whole article was written based on this bug, in which my colleague conducted research and figured out why this happened. Are you interested? There she is: “Scrollbar that failed“.
Second place: confused radius and height
And again we will talk about several analyzer warnings:
- V764 Possible incorrect order of arguments passed to ‘CreateWheel’ function: ‘height’ and ‘radius’. StandardJoints.cpp 791
- V764 Possible incorrect order of arguments passed to ‘CreateWheel’ function: ‘height’ and ‘radius’. StandardJoints.cpp 833
- V764 Possible incorrect order of arguments passed to ‘CreateWheel’ function: ‘height’ and ‘radius’. StandardJoints.cpp 884
Here are the function calls:
NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);And this is how her ad looks like:
static NewtonBody* CreateWheel (DemoEntityManager* const scene,
const dVector& location, dFloat radius, dFloat height)Arguments were reversed in function calls.
This error made it to the top from the article: “Rechecking Newton Game Dynamics with the PVS-Studio static analyzer“.
First place: Erasing the result
V519 The ‘color_name’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 621, 627. string.cpp 627
static bool parseNamedColorString(const std::string &value,
video::SColor &color)
{
std::string color_name;
std::string alpha_string;
size_t alpha_pos = value.find('#');
if (alpha_pos != std::string::npos) {
color_name = value.substr(0, alpha_pos);
alpha_string = value.substr(alpha_pos + 1);
} else {
color_name = value;
}
color_name = lowercase(value); // <=
std::map<const std::string, unsigned>::const_iterator it;
it = named_colors.colors.find(color_name);
if (it == named_colors.colors.end())
return false;
....
}This function should parse the color name with the transparency parameter and return its hexadecimal code. Depending on the result of checking the condition into a variable color_name either the result of the string splitting or a copy of the function argument is passed.
However, then in the function lowercase () not the resulting string itself is converted to lowercase, but the original argument of the function. As a result, we will simply lose the color that we should have returned. parseNamedColorString ()…
color_name = lowercase(color_name);This error made it to the top from the article: “PVS-Studio: Analysis of pull requests in Azure DevOps using self-hosted agents“.
Conclusion
Over the past year, we have found many bugs in open source projects. These were common copy-paste errors, constant errors, memory leaks, and many other problems. Our analyzer does not stand still, and in the top there are several detections of new diagnostics written this year.
Hope you enjoyed the collected bugs. Personally, I found them quite interesting. But, of course, your vision may differ from mine, so you can create your “Top 10 …” by reading articles from our blog or by looking at the list of errors found by PVS-Studio in open source projects.
I also bring to your attention articles with the top 10 C ++ errors of the past years: 2016, 2017, 2018, 2019…
If you want to share this article with an English-speaking audience, please use the translation link: Vladislav Stolyarov. Top 10 Bugs Found in C ++ Projects in 2020.
