Unicorn became interested in the microworld

This time, interesting examples of errors were presented to us by the microworld. We checked the open project μManager using the PVS-Studio code analyzer. This is a software package for automated microscope imaging.

μManager

This is a relatively small project. The source code is about 11 megabytes. What exactly this project is for, I do not know. I was asked to check it out. And now the unicorn is in a hurry to help. Probably a necessary and useful project, once asked.

Project site: Micro-Manager .

The analysis is as always done using the PVS-Studio analyzer. By the way, if you missed, then here is the comparison that our potential users have been waiting for: " Comparing code analyzers: CppCat, Cppcheck, PVS-Studio, Visual Studio ."
')
At this lyrical digression is over. Let's start looking at interesting code snippets.

long! = int

The μManager project claims to be cross-platform. Therefore, you need to be careful with the 'long' type. On 32-bit systems, the size of the 'long' type is the same as the size of the 'int' type. But in 64-bit systems it can be different. In Win64, the 'long' type remains 32-bit. In the 64-bit Linux world, another data model is adopted, in which the 'long' is 64-bit. You need to be vigilant using this type.

The μManager project contains the following bad code:

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 line (1) and (2), data is copied into variables of the type 'int'. Copies a number of bytes equal to the size of the 'long' type. Recall that in the 64-bit program the 'long' can occupy 8 bytes. And the type 'int' takes only 4 bytes.

In case (1) there is nothing to worry about. Change the value of the following members of the structure. Then these members will be filled again. Already correct.

But the case (2) is critical. The value of the last member changes. There will be a record for the redistribution of the structure. What this leads to depends on the luck and the phase of the moon.

Errors are detected due to the PVS-Studio diagnostic messages:

• V512 A call of the memcpy function will lead to overflow of the buffer & stat.lPosition. MotorStage.cpp 247
• V512 A call of the memcpy function will lead to overflow of the buffer & stat.dwStatusBits. MotorStage.cpp 256

Stop the garbage compactor!

 const unsigned char stopSgn[2] = {0x04, 0x66}; int MotorStage::Stop() { .... if (memcmp(stopSgn, answer, sizeof(stopSgn) != 0)) return ERR_UNRECOGNIZED_ANSWER; .... } 

The error is that the memcmp () function compares only one byte. Why? Offensive error. Not there is a closing bracket. The number of bytes for comparison is calculated as follows: sizeof (stopSgn)! = 0. This expression is equal to the value of 'true', which turns into one.

The condition should be:

 if (memcmp(stopSgn, answer, sizeof(stopSgn)) != 0) 

Error detected using diagnostics: V526 The 'memcmp' function returns 0 if the corresponding buffers are equal. Consider examining the condition for mistakes. MotorStage.cpp 385

Identical comparisons

 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 statement contains an invalid condition. What should be the second condition, I do not know. However, it is clearly seen that the second condition will never be fulfilled.

Diagnostics that detected the error: V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 1455, 1457. LeicaDMR.cpp 1455

There is another code fragment with a similar error. Apparently, with the installation of the position of a wheel there will be problems:

 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 diagnostic message: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a possibility of logical error presence. Check lines: 645, 654. Ludl.cpp 645

It seems we have forgotten something

I propose to look at this code here. Notice what is missing in it?

 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; } .... } 

Lacking a very important thing. Lost brackets (). The program should call the GetMotionMode () function and compare the value returned to it with zero. Instead, the address of the function is compared with zero.

The error was detected using the diagnostic: V516 Consider inspecting an odd expression. Non-null function pointer is compared to null: 'MP285 :: GetMotionMode == 0'. MP285ZStage.cpp 558

Lonely wanderer

 int HalogenLamp::SetIntensity(long intensity) { .... command_stream.str().c_str(); .... } 

What it is? Side effect of refactoring? Unfinished code? Harmless extra line or error?

There are two places where you can see such lonely wanderers:

• V530 The return value of the 'c_str' is required to be utilized. ZeissCAN.cpp 1553
• V530 The return value of the 'c_str' is required to be utilized. ZeissCAN.cpp 2800

Brahmins

 int LeicaScopeInterface::GetDICTurretInfo(....) { .... std::string tmp; .... if (tmp == "DIC-TURRET") scopeModel_->dicTurret_.SetMotorized(true); else scopeModel_->dicTurret_.SetMotorized(true); .... } 

This is what the Brahmin looks like. Regardless whether the condition is met or not, the same code is executed.

Warning: V523 The 'then' statement is equivalent to the 'else' statement. LeicaDMIScopeInterface.cpp 1296

Another mistake of a similar kind. Here the same strings are compared. Probably, this code contains a typo:

 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) .... } 

Warning: V549 The first argument of 'strcmp' function is equal to the second argument. XLedDev.cpp 119

Something does not fit

Values ​​of 'false' and 'true' can be implicitly cast to the type 'int':

• false becomes 0;
• true turns to 1.

For example, the following code will compile successfully:

 int F() { return false; } 

The F () function returns 0.

Sometimes people make mistakes, and instead of the error status, which is of type 'int', they return 'false' or 'true'. This happens because of forgetfulness. It's okay if the error status is encoded with a value of 0.

A trouble arises if error statuses are encoded with values ​​other than zero. That is what happens in the μManager project.

The following predefined values ​​are available:

 #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 .... 

Note that 0 means everything is fine. Other values ​​report some kind of error.

It seems to me that there is a confusion in the code of the μManager project with the statuses and values ​​of 'true', 'false'.

Consider the CreateProperty () function:

 int MM::PropertyCollection::CreateProperty(....) { if (Find(pszName)) return DEVICE_DUPLICATE_PROPERTY; .... if (!pProp->Set(pszValue)) return false; .... return DEVICE_OK; } 

Note that if the pProp-> Set (pszValue) call fails, the function returns 'false'. It turns out that the function returns the status DEVICE_OK. It is very strange.

Another suspicious code snippet:

 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; } 

At the end, we see "return true;". This means that the function will return the status DEVICE_ERR 1 (generic, undefined error). At the same time, it seems to me that everything is really good.

It may seem strange to read, why I call these places suspicious, rather than saying that these are errors. The fact is that in places 'false' is used specifically to highlight special cases. 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 } .... } 

Pay attention to the comment. An error has occurred. But we pretend that everything is good, returning zero. Perhaps 'false' is written instead of DEVICE_OK to emphasize the peculiarity of this code.

There are only a couple of such comments. But in other places it is not clear if this is a mistake or “sly feint with ears”. I would venture to suggest that in half the places everything is correct, and half really turn out to be mistakes.



In any case, this code smells really bad.

Here is a list of all suspicious sites:

• V601 The 'false' value is an implicitly casted to the integer type. Property.cpp 364
• V601 The 'true' value is implicitly casted to the integer type. Property.cpp 464
• V601 The 'false' value is an implicitly casted to the integer type. PIGCSControllerCom.cpp 405
• V601 The 'false' value is an implicitly casted to the integer type. Prior.cpp 778
• V601 The 'false' value is an implicitly casted to the integer type. Prior.cpp 2308
• V601 The 'false' value is an implicitly casted to the integer type. Prior.cpp 2313
• V601 The 'false' value is an implicitly casted to the integer type. Prior.cpp 2322
• V601 The 'false' value is an implicitly casted to the integer type. SutterLambda.cpp 190
• V601 The 'false' value is an implicitly casted to the integer type. SutterLambda.cpp 269
• V601 The 'false' value is an implicitly casted to the integer type. SutterLambda.cpp 285
• V601 The 'false' value is an implicitly casted to the integer type. Tofra.cpp 900
• V601 The 'false' value is an implicitly casted to the integer type. Tofra.cpp 1806
• V601 The 'false' value is an implicitly casted to the integer type. Tofra.cpp 1830

Weird get

 int pgFocus::GetOffset(double& offset) { MM_THREAD_GUARD_LOCK(&mutex); deviceInfo_.offset = offset; MM_THREAD_GUARD_UNLOCK(&mutex); return DEVICE_OK; } 

It seems to me, or is there something wrong with this code?

The analyzer does not like this code: V669 The 'offset' argument is a non-constant reference. The analyzer is unable to determine where this argument is being modified. It is possible that the function contains an error. pgFocus.cpp 356

And indeed, strange. The function is called "Get____". The function returns the status. It also takes the 'offset' argument by reference. And ... And does not write anything into it. I do not know how it all works. But perhaps it was necessary to do the assignment in reverse? Something like this:

 offset = deviceInfo_.offset; 

Another suspicious GetTransmission () function:

 int SpectralLMM5Interface::GetTransmission(...., double& transmission) { .... int16_t tr = 0; memcpy(&tr, answer + 1, 2); tr = ntohs(tr); transmission = tr/10; .... } 

PVS-Studio warning: V636 The 'tr / 10' expression was implicitly casted from 'int' type to 'double' type. Consider using a fractional part. An example: double A = (double) (X) / Y ;. SpectralLMM5Interface.cpp 198

Please note that the return value is of type double (we are talking about transmission). But this value is calculated strange. The integer value is divided by 10. I have a strong suspicion that a loss of precision occurs. For example, if 'tr' is 5, then after division we get 0, and not at all 0.5.

Probably the correct code should look like this:

 transmission = tr/10.0; 

Error or not error? The first impression can be deceptive.

In C / C ++, numbers starting with zero are considered to be given in octal format. There is one suspicious place in the μManager project:

 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 warning: V536 Be advised that it is not possible. Oct: 010, Dec: 8. LeicaDMSTCHub.cpp 142

It is not clear, really wanted to use the number 8, written in octal format, or this is a mistake. In other places, the numbers written in the decimal number system are passed to the SetCommand () function. For example:

 int ret = SetCommand(device, core, xyStage_, 35, ack); 

I do not know if an error was found or not, but it is worth mentioning this place.

Perfectionist indignant

There are lots of little things that are not essential. However, almost all programmers are perfectionists. Let's grumble.

Full of extra lines. One 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 clearly superfluous.

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; .... } 

The second check again does not make sense. Before it, the variable 'ret' will not change anywhere. The second check can be safely removed.

There are a lot of such extra checks. I will give them a list: Micro-Manager-V571-V649.txt .

Even from the little things can be noted the wrong format when working with functions sprintf (). The unsigned variable is printed as signed. This can lead to incorrect printing of large values.

 int MP285Ctrl::Initialize() { .... unsigned int nUm2UStepUnit = MP285::Instance()->GetUm2UStep(); .... sprintf(sUm2UStepUnit, "%d", nUm2UStepUnit); .... } 

There were three such places:

• V576 Incorrect format. Consider checking the sprintf function. The SIGNED integer type argument is expected. MP285Ctrl.cpp 253
• V576 Incorrect format. Consider checking the sprintf function. The SIGNED integer type argument is expected. MP285Ctrl.cpp 276
• V576 Incorrect format. Consider checking the sprintf function. The SIGNED integer type argument is expected. MP285Ctrl.cpp 327

Conclusion

A single check of this and any other project is ineffective. Benefit can be obtained only with regular use of static code analyzers. Then many errors and typos will be corrected at an early stage. Consider static analysis as an extension of the warnings that the compiler issues.

For teams creating medium and large projects for the Windows operating system, we recommend using our static analyzer PVS-Studio . The price depends on the size of the team and the required level of support.

For small teams and individual developers, we offer the CppCat tool. Individual license - $ 250. Renewal - $ 200. When buying multiple licenses - discounts.

For those who work with Linux, we suggest paying attention to the free Cppcheck code analyzer . Or try the Standalone version of PVS-Studio.

PS

Translation of this article: " The Unicorn's Travel to the Microcosm ".