While the world is discussing the 89th Ceremony of Oscar award and charts of actors and costumes, we've decided to write a review article about the IT-sphere. The article is going to cover the most interesting bugs, made in open source projects in 2016. This year was remarkable for our tool, as PVS-Studio has become available on Linux OS. The errors we present are hopefully, already fixed, but every reader can see how serious are the errors made by developers.
So, let's see, which bugs PVS-Studio analyzer managed to find in 2016. Besides the code fragment, we provide a diagnostic, which helped to detect the error and the article, where this error was first described.
The sections are sorted according to my idea of the error beauty.
Tenth place
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 pointer bb_copy is set to nil twice, and the pointer bb_original remains the same.
Ninth place
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;
}
A nasty typo that was quickly fixed after the article was posted. By the way, this erroneous code was copied several times to different fragments of the project. The analyzer found them too.
Eighth place
V579 The read_memory function receives the pointer and its size as arguments. 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 evaluates not the buffer size, but the pointer size. Therefore, the program doesn't get enough bytes of data.
Seventh place
Source: PVS-Studio team is about to produce a technical breakthrough, but for now let's recheck Blender
V522 Dereferencing of the null pointer 've' 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;
}
....
}
The typo in the names had consequences that are more serious. Apparently, the second code fragment was written using Copy-Paste. By accident, the programmer forgot to change the variable name ve with fe. As a result, we will have an undefined behavior, which may lead to a crash, for example.
Sixth place
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 bug in the list of class initialization. The field m_subld is initialized by itself; perhaps the programmer wanted to write m_subId(subId).
Fifth place
V603 The object was created but it is not being used. If you wish to call 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();
....
}
It is a rare bug, but some programmers still think that such a call of a constructor initializes the fields of a class. Instead of accessing the current object, a new temporary object is created and then immediately destroyed. As a result, the fields of the object will not be initialized. If you need to use field initialization outside the constructor, it is better to create a separate function and access it.
Fourth place
Source: Casablanca: The Little Unicorn That Could
V554 Incorrect use of shared_ptr. The memory 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, the smart pointer of the shared_ptr type for destruction of an object will call the delete operator without the brackets []. In this case, it is wrong.
The correct code should be:
std::shared_ptr<int> ss(new int[decks * 52],
std::default_delete<int[]>());Third place
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); // <=
....
}
Some string is formed in the buffer, and then the programmer wants to get a new string, saving the previous string value and add two more words. It seems really simple.
To explain why unexpected result can be received here, I will quote a simple and clear example from the documentation for the diagnostic V541:
char s[100] = "test";
sprintf(s, "N = %d, S = %s", 123, s);
As a result we would want to have a string:
N = 123, S = test
But in practice, we will have such a string in the buffer:
N = 123, S = N = 123, S =
It is hard to say what will happen in our case, because it depends on the implementation of the sprintf function. There is a chance that the code will work in the way it is expected. But we may also get an incorrect variant or a program crash. The code can be fixed if you use a new buffer to store the result .
Second place
V733 It is possible that macro expansion resulted 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 strange in this code fragment. We see that sometimes the 'chan' value is used, sometimes less by one 'chan - 1', but let us have look at the macro definition:
#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 the binary expression to the macro, the evaluation logic changes dramatically. The expression "(chan - 1) * 20" turns into "chan - 1 *20", i.e. into "chan - 20", and the incorrectly evaluated size gets used further in the program.
Unfortunately, this error has not been fixed yet. Perhaps, the developers did not notice it in the article or have not fixed yet, but the code still looks strange. That is why FreeBSD took the second award.
First place
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);
}
....
}
The first place of this rating of 2016 is taken by VirtualBox project. It was checked by PVS-Studio several times and each time we detected a large number of errors. However, this error was so confusing that it misled not only the author of the code but even us, the developers of the analyzer. We really had to think a lot what was wrong with the code and why PVS-Studio issued such a strange warning.
In the compiled code in Windows we saw the replacement of functions. A new function returned a value of unsigned type, adding almost an invisible error. Here are the prototypes of the functions:
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 wanted to show the most popular picture that got a lot of enthusiastic comments. A picture from the article "PVS-Studio checked OpenJDK"
Now anybody can offer projects for a check via Github on Windows and Linux, which will help us find more errors in open source projects and improve the quality of these projects.
You may download and try PVS-Studio by this link.
In case you want to discuss the licensing options, prices and discounts, contact us at the support.
We wish you bugless coding!
No comments:
Post a Comment