Top 10 errors in open projects With ++ for 2016

While all over the world they are discussing the 89th Oscar awards ceremony and are making different ratings of actors and their costumes, we decided to prepare an overview article in the IT sphere. It will focus on the most interesting mistakes made in open source projects in 2016. This year was notable for the fact that our PVS-Studio analyzer became available in Linux-based operating systems. The errors presented are probably already corrected, and every reader can be convinced of the seriousness of the mistakes that developers make.

So, let's consider what interesting errors the PVS-Studio analyzer found in 2016. In addition to the code, the diagnosis will be given, with the help of which the error was detected, as well as the article in which this error was first described by us.

Sections are sorted according to my ideas about the beauty of bugs :).

Tenth place

Source: We find errors in the GCC compiler code using the PVS-Studio analyzer
')
V519 The 'bb_copy' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1076, 1078. cfg.c 1078

void free_original_copy_tables (void) { gcc_assert (original_copy_bb_pool); delete bb_copy; bb_copy = NULL; // <= delete bb_original; // <= bb_copy = NULL; // <= delete loop_copy; loop_copy = NULL; delete original_copy_bb_pool; original_copy_bb_pool = NULL; } 

The bb_copy pointer is reset by chance twice, and the bb_original pointer remains unquoted.

Ninth place

Source: CryEngine V Welcome Check

V519 The 'BlendFactor [2]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266

 void CCryDXGLDeviceContext:: OMGetBlendState(...., FLOAT BlendFactor[4], ....) { CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState); if ((*ppBlendState) != NULL) (*ppBlendState)->AddRef(); BlendFactor[0] = m_auBlendFactor[0]; BlendFactor[1] = m_auBlendFactor[1]; BlendFactor[2] = m_auBlendFactor[2]; // <= BlendFactor[2] = m_auBlendFactor[3]; // <= *pSampleMask = m_uSampleMask; } 

An annoying grief, which promptly corrected after the publication of the article. By the way, this code was copied several times along with an error in different places of the project. Their analyzer also found.

Eighth place

Source: GDB was a tough nut to crack.

V579 It is possibly a mistake. Inspect the third argument. jv-valprint.c 111

 extern void read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len); void java_value_print (....) { .... gdb_byte *buf; buf = ((gdb_byte *) alloca (gdbarch_ptr_bit (gdbarch) / HOST_CHAR_BIT)); .... read_memory (address, buf, sizeof (buf)); .... } 

The sizeof (buf) operator does not calculate the size of the buffer, but the size of the pointer. Therefore, not enough data bytes are extracted from the memory.

Seventh place

Source: PVS-Studio team is preparing a technical breakthrough, but for now we will double-check Blender

V522 Dereferencing of the null pointer 'might take place. functions1d.cpp 107

 int QuantitativeInvisibilityF1D::operator()(....) { ViewEdge *ve = dynamic_cast<ViewEdge*>(&inter); if (ve) { result = ve->qi(); return 0; } FEdge *fe = dynamic_cast<FEdge*>(&inter); if (fe) { result = ve->qi(); // <= return 0; } .... } 

Here a typo in the name led to a more serious error. Apparently, the second code snippet was written using Copy-Paste. And by chance in one place they forgot to change the variable name ve to fe . As a result, an undefined program behavior will occur, which may, for example, lead to a program crash.

Sixth place

Source: Bad Package Code for Toonz 2D Animations

V546 Member of a class is initialized by itself: 'm_subId (m_subId)'. tfarmcontroller.cpp 572

 class TaskId { int m_id; int m_subId; public: TaskId(int id, int subId = -1) : m_id(id), m_subId(m_subId){}; 

An interesting error in the class initialization list. The m_subld field is initialized by itself, although, most likely, they wanted to write m_subId (subId) .

Fifth place

Source: PVS-Studio hurries to the aid of CERN: checking the project Geant4

V603 The object was not used. If you wish to call the constructor, 'this-> G4PhysicsModelCatalog :: G4PhysicsModelCatalog (....)' should be used. g4physicsmodelcatalog.cc 51

 class G4PhysicsModelCatalog { private: .... G4PhysicsModelCatalog(); .... static modelCatalog* catalog; .... }; G4PhysicsModelCatalog::G4PhysicsModelCatalog() { if(!catalog) { static modelCatalog catal; catalog = &catal; } } G4int G4PhysicsModelCatalog::Register(const G4String& name) { G4PhysicsModelCatalog(); .... } 

A rare mistake, but some programmers still think that such a call to the constructor initializes the class fields. Instead of referring to the current object, a new temporary object is created, which will be immediately destroyed. As a result, the object fields will not be initialized. When it is required to use field initialization outside the constructor, it is better to create a separate function for this and access it.

Fourth place

Source: Casablanca: Unicorn that could

V554 Incorrect use of shared_ptr. The memory is allocated with 'new []' will be cleaned using 'delete'. BlackJack_Server140 table.cpp 471

 void DealerTable::FillShoe(size_t decks) { std::shared_ptr<int> ss(new int[decks * 52]); .... } 

By default, a smart pointer such as shared_ptr to invoke an object will invoke the delete operator without square brackets []. In this case, it is wrong.

The correct code should be as follows:

 std::shared_ptr<int> ss(new int[decks * 52], std::default_delete<int[]>()); 

Third place

Source: Checking the source code of the game engine Serious Engine v.1.10 for the anniversary of the shooter Serious Sam

V541 It is dangerous to print the string 'achrDefaultScript' into itself. dlgcreateanimatedtexture.cpp 359

 BOOL CDlgCreateAnimatedTexture::OnInitDialog() { .... // allocate 16k for script char achrDefaultScript[ 16384]; // default script into edit control sprintf( achrDefaultScript, ....); // <= .... // add finishing part of script sprintf( achrDefaultScript, // <= "%sANIM_END\r\nEND\r\n", // <= achrDefaultScript); // <= .... } 

Here form a certain line in the buffer. Then they want to get a new line, keeping the previous value of the line, and add two more words to it. It seems simple.

To explain why an unexpected result can be obtained here, I will quote a simple example from the V541 diagnostic documentation:

 char s[100] = "test"; sprintf(s, "N = %d, S = %s", 123, s); 

As a result of this code, I want to get the line

 N = 123, S = test 

But in practice, the string will be formed in the buffer:

 N = 123, S = N = 123, S = 

What will produce in our case is difficult to predict, since it depends on the implementation of the sprintf function. There is a chance that the code will work as the programmer expected. And you can get an incorrect version or the fall of the program. The code can be corrected by using a new buffer to save the result.

Second place

Source: PVS-Studio rummaged in the FreeBSD kernel

V733 It is possible that macro expansion in incorrect evaluation order. Check expression: chan - 1 * 20. isp.c 2301

 static void isp_fibre_init_2400(ispsoftc_t *isp) .... if (ISP_CAP_VP0(isp)) off += ICB2400_VPINFO_PORT_OFF(chan); else off += ICB2400_VPINFO_PORT_OFF(chan - 1); // <= .... } 

At first glance, there is nothing suspicious in this code snippet. Sometimes the value 'chan' is used, sometimes one less: 'chan - 1', but let's look at the definition of a macro:

 #define ICB2400_VPOPT_WRITE_SIZE 20 #define ICB2400_VPINFO_PORT_OFF(chan) \ (ICB2400_VPINFO_OFF + \ sizeof (isp_icb_2400_vpinfo_t) + \ (chan * ICB2400_VPOPT_WRITE_SIZE)) // <= 

When passing a binary expression to a macro, the calculation logic changes drastically there. The supposed expression "(chan - 1) * 20" turns into "chan - 1 * 20", i.e. in “chan - 20”, and then the wrong size is used in the program.

Unfortunately, this error has not yet been fixed. Perhaps the developers did not notice it in the article or did not find the time to fix it, but the code still looks erroneous. Therefore, FreeBSD almost leads the error rating.

First place

Source: A fresh look at the Oracle VM VirtualBox code

V547 Expression is always false. Unsigned type value is never <0. dt_subr.c 715

 #define vsnprintf RTStrPrintfV int dt_printf(dtrace_hdl_t *dtp, FILE *fp, const char *format, ...) { .... if (vsnprintf(&dtp->dt_buffered_buf[dtp->dt_buffered_offs], // <= avail, format, ap) < 0) { rval = dt_set_errno(dtp, errno); va_end(ap); return (rval); } .... } 

He heads the ranking of the most interesting mistakes in the 2016 VirtualBox project. It was repeatedly tested using PVS-Studio, a lot of errors were detected. But this error misled not only the author of the code, but also made even us, the developers of the analyzer, take a long time to understand what is wrong with this code and why PVS-Studio gives a strange warning.

In the compiled code for Windows, function substitution occurred. The new function returned the value of an unsigned type, adding an almost invisible error to the code. Here are the prototypes of the functions used:

 size_t RTStrPrintfV(char *, size_t, const char *, va_list args); int vsnprintf (char *, size_t, const char *, va_list arg ); 

Conclusion

In conclusion, I want to bring the most popular image to the article, which received many enthusiastic comments. Picture from the article " Checking an OpenJDK project with PVS-Studio ":

Now any of you can offer projects for review through GitHub for Windows or Linux, which will allow us to find even more errors in open source projects and make them more qualitative.

Download and try PVS-Studio at this link.

Discuss licensing options, prices and options for discounts you can write to us in support .

I wish you all a reckless code!



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Top 10 bugs in C ++ open source projects, checked in 2016