This time it was the microcosm that brought us a few interesting bugs. We have checked the open-source project μManager with our analyzer PVS-Studio. This project is a software package for automated microscope image acquisition.
It is a relatively small project with the source code's size about 11 Mbytes. I don't know what it does exactly, I just was asked to check it - and here's our unicorn coming to help. But I guess it is a very useful and important project since people wanted it checked.
The project website: Micro-Manager.
As usual, analysis was done with the PVS-Studio static analyzer. By the way, in case you have missed it - we carried out a large comparison our potential customers had been waiting for for a long time, welcome to check it: "Comparison of static code analyzers: CppCat, Cppcheck, PVS-Studio and Visual Studio".
Let's finish with this parenthetical remark here and go on with studying the interesting code fragments we have found.
The μManager project is claiming to be crossplatform. In this connection, the authors should be careful with the 'long' type. In 32-bit systems, the size of the 'long' type coincides with that of the 'int' type. But things may turn different in 64-bit systems. Thus, the 'long' type remains 32-bit in Win64 but gets 64-bit in the 64-bit Linux world where another data model is supported. That's why one should be very careful when using this type.
Here's an example of a poor code fragment in the μManager project:
typedef struct _DCMOTSTATUS
{
unsigned short wChannel; // Channel ident.
unsigned int lPosition; // Position in encoder counts.
unsigned short wVelocity; // Velocity in encoder counts/sec.
unsigned short wReserved; // Controller specific use
unsigned int dwStatusBits; // Status bits (see #defines below).
} DCMOTSTATUS;
int MotorStage::ParseStatus(...., DCMOTSTATUS& stat)
{
....
memcpy(&stat.lPosition, buf + bufPtr, sizeof(long)); //<<<(1)
bufPtr += sizeof(long);
memcpy(&stat.wVelocity, buf + bufPtr, sizeof(unsigned short));
bufPtr += sizeof(unsigned short);
memcpy(&stat.wReserved, buf + bufPtr, sizeof(unsigned short));
bufPtr += sizeof(unsigned short);
memcpy(&stat.dwStatusBits,
buf + bufPtr, sizeof(unsigned long)); //<<<(2)
return DEVICE_OK;
}In the lines (1) and (2), data are copied into variables of the 'int' type. The numbers of bytes being copied is equal to the size of the 'long' type. But one should keep in mind that 'long' may occupy 8 bytes in a 64-bit program, while 'int' occupies only 4 bytes.
No serious trouble will occur in the line (1). We can change the values of the following structure members and then they will be filled once more, and that piece will become correct.
But the line (2) has a critical issue. The value of the last member is changed, which will cause writing outside the structure boundaries. Its consequences depend on luck and phase of the moon.
PVS-Studio relied on the following diagnostic messages to detect these errors:
const unsigned char stopSgn[2] = {0x04, 0x66};
int MotorStage::Stop()
{
....
if (memcmp(stopSgn, answer, sizeof(stopSgn) != 0))
return ERR_UNRECOGNIZED_ANSWER;
....
}The error is this: the memcmp() function compares only one byte. Why? A sad mistake it is - a closing parenthesis is written in a wrong place. The number of bytes to be compared is calculated in the following way: sizeof(stopSgn) != 0. This expression evaluates to 'true', which then turns to one.
The condition should look like this:
if (memcmp(stopSgn, answer, sizeof(stopSgn)) != 0)PVS-Studio's diagnostic message: V526 The 'memcmp' function returns 0 if corresponding buffers are equal. Consider examining the condition for mistakes. MotorStage.cpp 385
const char* g_Out = "Out";
int FieldDiaphragm::OnCondensor(....)
{
....
std::string value;
....
if (value == g_Out)
return
g_hub.SetCondensorPosition(*this, *GetCoreCallback(), 0);
else if (value == g_Out)
return
g_hub.SetCondensorPosition(*this, *GetCoreCallback(), 1);
....
}The second 'if' operator contains an incorrect condition. I don't know for sure what exactly it should look like, but it obviously will never be true if left as it is.
PVS-Studio's diagnostic message: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1455, 1457. LeicaDMR.cpp 1455
There is one more code fragment with a similar error. I guess some wheel in the microscope will not work properly:
class Wheel : public CStateDeviceBase<Wheel>
{
....
unsigned wheelNumber_;
....
};
int Wheel::SetWheelPosition(int position)
{
unsigned char cmd[4];
cmd[0] = moduleId_; cmd[2] = 0; cmd[3] = 58;
if (wheelNumber_ == 1) {
switch (position) {
case 0: cmd[1] = 49; break;
case 1: cmd[1] = 50; break;
case 2: cmd[1] = 51; break;
case 3: cmd[1] = 52; break;
case 4: cmd[1] = 53; break;
case 5: cmd[1] = 54; break;
}
} else if (wheelNumber_ == 1) {
switch (position) {
case 0: cmd[1] = 33; break;
case 1: cmd[1] = 64; break;
case 2: cmd[1] = 35; break;
case 3: cmd[1] = 36; break;
case 4: cmd[1] = 37; break;
case 5: cmd[1] = 94; break;
}
....
}PVS-Studio's diagnostic message: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 645, 654. Ludl.cpp 645
Take a look at the following code. Will you notice what is missing?
class MP285
{
....
static int GetMotionMode() { return m_nMotionMode; }
....
};
int ZStage::_SetPositionSteps(....)
{
....
if (MP285::GetMotionMode == 0)
{
long lOldZPosSteps = (long)MP285::Instance()->GetPositionZ();
dSec = (double)labs(lZPosSteps-lOldZPosSteps) / dVelocity;
}
else
{
dSec = (double)labs(lZPosSteps) / dVelocity;
}
....
}It is actually a very important thing which is missing - the parentheses (). The program must call the function GetMotionMode() and compare its return value to zero. Instead, it is the function address that will be compared to zero.
PVS-Studio's diagnostic message: V516 Consider inspecting an odd expression. Non-null function pointer is compared to null: 'MP285::GetMotionMode == 0'. MP285ZStage.cpp 558
int HalogenLamp::SetIntensity(long intensity)
{
....
command_stream.str().c_str();
....
}What is it? A side effect of refactoring? Incomplete code? A harmless odd line? A mistake?
Such lone wanderers can be found in two fragments:
int LeicaScopeInterface::GetDICTurretInfo(....)
{
....
std::string tmp;
....
if (tmp == "DIC-TURRET")
scopeModel_->dicTurret_.SetMotorized(true);
else
scopeModel_->dicTurret_.SetMotorized(true);
....
}This is what a code "brahmin" looks like. Regardless of whether or not the condition is true, one and the same code branch will be executed.
PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. LeicaDMIScopeInterface.cpp 1296
Here's another similar error: identical strings are compared. This code seems to have a typo somewhere:
int XLedDev::Initialize()
{
....
if (strcmp(
XLed::Instance()->GetXLedStr(XLed::XL_WLedDevName +
m_nLedDevNumber).c_str(),
XLed::Instance()->GetXLedStr(XLed::XL_WLedDevName +
m_nLedDevNumber).c_str()
) != 0)
....
}PVS-Studio's diagnostic message: V549 The first argument of 'strcmp' function is equal to the second argument. XLedDev.cpp 119
The values 'false' and 'true' can be implicitly cast to the 'int' type:
For example, the following code is well compilable:
int F() { return false; }The F() function returns 0.
Sometimes programmers may confuse things and make mistakes which cause functions to return 'false' or 'true' instead of an error status code of the 'int' type. It's not crucial when the error status is coded by 0.
Trouble occurs when error statuses are coded by values other than zero. And this is what happens in the μManager project.
There are the following predefined values:
#define DEVICE_OK 0
#define DEVICE_ERR 1 // generic, undefined error
#define DEVICE_INVALID_PROPERTY 2
#define DEVICE_INVALID_PROPERTY_VALUE 3
#define DEVICE_INVALID_PROPERTY_TYPE 5
....Notice that 0 means everything's alright; any other value indicates some error.
I suspect programmers messed something up with error statuses and true/false values in μManager.
Take a look at the function CreateProperty():
int MM::PropertyCollection::CreateProperty(....)
{
if (Find(pszName))
return DEVICE_DUPLICATE_PROPERTY;
....
if (!pProp->Set(pszValue))
return false;
....
return DEVICE_OK;
}Note that if the call pProp->Set(pszValue) is executed without success, the function returns 'false'. That is, it appears to return the DEVICE_OK status, which is very strange.
Another suspicious code fragment:
int MM::PropertyCollection::RegisterAction(
const char* pszName, MM::ActionFunctor* fpAct)
{
MM::Property* pProp = Find(pszName);
if (!pProp)
return DEVICE_INVALID_PROPERTY;
pProp->RegisterAction(fpAct);
return true;
}There is the line "return true;" at the end, which means that the function will return the status DEVICE_ERR 1 (generic, undefined error). However, everything seems alright actually.
Perhaps you find it strange that I call such fragments suspicious, not definitely bugs. You see, 'false' is sometimes used deliberately to point out some special cases. For example:
int XYStage::Home()
{
....
if (ret != DEVICE_OK)
{
ostringstream os;
os << "ReadFromComPort failed in "
"XYStage::Busy, error code:" << ret;
this->LogMessage(os.str().c_str(), false);
return false; // Error, let's pretend all is fine
}
....
}Note the comment: an error occurred, but we'll pretend all is fine and return zero. Perhaps 'false' was consciously chosen to be returned instead of DEVICE_OK to stress that it is a special code fragment.
However, there are pretty few comments of that kind. And for all the rest fragments, I can't say for sure if it is an error or a cunning trick. I'll risk assuming that half of them are correct and half are not.
Anyway, this code smells quite a bit.
Here is a list of all the suspicious fragments of this kind:
int pgFocus::GetOffset(double& offset)
{
MM_THREAD_GUARD_LOCK(&mutex);
deviceInfo_.offset = offset;
MM_THREAD_GUARD_UNLOCK(&mutex);
return DEVICE_OK;
}It only seems so, or something is really wrong with this code?
The analyzer doesn't like it: V669 The 'offset' argument is a non-constant reference. The analyzer is unable to determine the position at which this argument is being modified. It is possible that the function contains an error. pgFocus.cpp 356
And that's strange indeed. The function is named "Get____" but returns a status code. Besides, it also receives the 'offset' argument by reference... and doesn't write anything into it. I don't know how it all works, but I think that assignment should have been done vice versa - something like this:
offset = deviceInfo_.offset;One more suspicious function GetTransmission():
int SpectralLMM5Interface::GetTransmission(....,
double& transmission)
{
....
int16_t tr = 0;
memcpy(&tr, answer + 1, 2);
tr = ntohs(tr);
transmission = tr/10;
....
}PVS-Studio's diagnostic message: V636 The 'tr / 10' expression was implicitly casted from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. SpectralLMM5Interface.cpp 198
Notice that the return value (transmission) is of the double type. But it is calculated in a strange way: an integer value is divided by 10. I'm almost sure this operation will cause a loss of accuracy. For example, if 'tr' equals 5, we'll get 0 instead of 0.5 after division.
Perhaps the correct code should look like this:
transmission = tr/10.0;In the C/C++ language, numbers starting with zero are treated as octal numbers. There is one suspicious piece of code in μManager:
int LeicaDMSTCHub::StopXY(MM::Device& device, MM::Core& core)
{
int ret = SetCommand(device, core, xyStage_, 010);
if (ret != DEVICE_OK)
return ret;
return DEVICE_OK;
}PVS-Studio's diagnostic message: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 010, Dec: 8. LeicaDMSTCHub.cpp 142
It's not clear if the programmer really intended to use number 8 in the octal form or this is just a mistake. In other lines, the SetCommand() function receives decimal numbers. For example:
int ret = SetCommand(device, core, xyStage_, 35, ack);I don't know if this is an error but the fragment is worth mentioning.
There is a pile of small nuances which are not crucial to the program operation. But since most programmers are perfectionists, I can't help grumbling a little.
Lots of unnecessary lines. For example:
int XYStage::OnTriggerEndX(MM::PropertyBase* pProp,
MM::ActionType eAct){
if (eAct == MM::BeforeGet)
{
int ret = GetCommandValue("trgse",xChannel_,chx_.trgse_);
if (ret!=DEVICE_OK)
if (ret!=DEVICE_OK)
return ret;
.....
}The second check is obviously redundant.
Another example:
int AFC::Initialize()
{
int ret = DEVICE_OK;
....
if (ret != DEVICE_OK)
return ret;
AddAllowedValue("DichroicMirrorIn", "0", 0);
AddAllowedValue("DichroicMirrorIn", "1", 1);
if (ret != DEVICE_OK)
return ret;
....
}Again, the second check makes no sense. The 'ret' variable before it will not be changed anywhere, so this check can be removed.
There are quite a lot of such redundant checks, so here you are a full list: Micro-Manager-V571-V649.txt.
Among other trifles like that I can name incorrect handling of sprintf() functions when unsigned variables are printed as signed ones. It may cause incorrect printing of large values.
int MP285Ctrl::Initialize()
{
....
unsigned int nUm2UStepUnit = MP285::Instance()->GetUm2UStep();
....
sprintf(sUm2UStepUnit, "%d", nUm2UStepUnit);
....
}We found three fragments with this error:
A one-time check of this or any other project is not efficient and not enough. You can only benefit from static analysis when using it regularly - then you will be able to catch and fix most mistakes and typos at the earliest development stage. Treat static analysis as an extension of compiler-generated warnings.
We recommend all the teams working on medium and large projects under Windows to try our static analyzer PVS-Studio. Its price depends on the team size and the level of support the team needs.
Those who work under Linux can try the free code analyzer Cppcheck or the Standalone version of PVS-Studio.