# Единорог заинтересовался микромиром В этот раз интересные примеры ошибок нам преподнёс микромир\. Мы проверили с помощью анализатора кода PVS\-Studio открытый проект μManager\. Это программный пакет для автоматизированного получения изображения с микроскопа\. ![0242_Travel_to_Microcos_ru/image1.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image1.png) ## μManager Это относительно небольшой проект\. Объем исходного кода около 11 мегабайт\. Для чего именно нужен этот проект, я не знаю\. Меня попросили его проверить\. И вот единорог уже спешит на помощь\. Наверное, нужный и полезный проект, раз попросили\. Сайт проекта: [Micro\-Manager](https://micro-manager.org/)\. Анализ как всегда выполнен с помощью анализатора [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Кстати, если вы пропустили, то вот сравнение, которое так долго ждали наши потенциальные пользователи: "[Сравнение анализаторов кода: CppCat, Cppcheck, PVS\-Studio, Visual Studio](https://pvs-studio.ru/ru/blog/posts/0241/)"\. На этом лирическое отступление окончено\. Начнём рассматривать интересные фрагменты кода\. ## long \!\= int ![0242_Travel_to_Microcos_ru/image2.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image2.png) Проект μManager претендует на кроссплатформенность\. Поэтому, надо быть аккуратным с типом 'long'\. В 32\-битных системах размер типа 'long' совпадает с размером типа 'int'\. А вот в 64\-битных системах может быть по\-разному\. В Win64 тип 'long' остался 32\-битным\. В 64\-битном мире Linux принята другая [модель данных](https://pvs-studio.ru/ru/blog/terms/0012/), в которой 'long' является 64\-битным\. Нужно проявлять бдительность, используя этот тип\. Проект μManager содержит следующий неудачный код: ```cpp 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; } ``` В строке \(1\) и \(2\) происходит копирование данных в переменные, имеющие тип 'int'\. Копируется количество байт, равное размеру типа 'long'\. Вспомним, что в 64\-битной программе 'long' может занимать 8 байт\. А тип 'int' занимает только 4 байта\. В случае \(1\) в этом нет ничего страшного\. Изменим значение следующих членов структуры\. Дальше эти члены заполнятся ещё раз\. Уже правильно\. А вот случай \(2\) критичен\. Изменяется значение последнего члена\. Произойдёт запись за переделами структуры\. К чему это приведёт, зависит от везения и фазы луны\. Ошибки выявляются благодаря диагностическим сообщениям PVS\-Studio: * 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 ## Останови уплотнитель мусора\! ![0242_Travel_to_Microcos_ru/image3.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image3.png) ```cpp const unsigned char stopSgn[2] = {0x04, 0x66}; int MotorStage::Stop() { .... if (memcmp(stopSgn, answer, sizeof(stopSgn) != 0)) return ERR_UNRECOGNIZED_ANSWER; .... } ``` Ошибка в том, что функция memcmp\(\) сравнивает только один байт\. Почему? Обидная ошибка\. Не там поставлена закрывающаяся скобка\. Количество байт для сравнения вычисляется так: sizeof\(stopSgn\) \!\= 0\. Это выражение равно значению 'true', которое превращается в единицу\. Условие должно быть таким: ```cpp if (memcmp(stopSgn, answer, sizeof(stopSgn)) != 0) ``` Ошибка выявлена с помощью диагностики: V526 The 'memcmp' function returns 0 if corresponding buffers are equal\. Consider examining the condition for mistakes\. MotorStage\.cpp 385 ## Идентичные сравнения ![0242_Travel_to_Microcos_ru/image4.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image4.png) ```cpp 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); .... } ``` Второй оператор 'if' содержит неправильное условие\. Каким должно быть второе условие, я не знаю\. Однако, хорошо видно, что второе условие никогда не выполнится\. Диагностика, выявившая ошибку: 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 Есть ещё один фрагмент кода с аналогичной ошибкой\. Видимо, с установкой позиции какого\-то колёсика будут проблемы: ```cpp class Wheel : public CStateDeviceBase { .... 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: 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 ## Кажется, мы о чём\-то забыли ![0242_Travel_to_Microcos_ru/image5.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image5.png) Предлагаю посмотреть вот на этот код\. Заметите, чего в нём не хватает? ```cpp 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; } .... } ``` Не хватает очень важной вещи\. Забыты скобочки \(\)\. Программа должна вызывать функцию GetMotionMode\(\) и сравнивать возвращаемое ей значение с нулём\. Вместо этого с нулём сравнивается адрес функции\. Ошибка была обнаружена с помощью диагностики: V516 Consider inspecting an odd expression\. Non\-null function pointer is compared to null: 'MP285::GetMotionMode \=\= 0'\. MP285ZStage\.cpp 558 ## Одинокой странник ![0242_Travel_to_Microcos_ru/image6.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image6.png) ```cpp int HalogenLamp::SetIntensity(long intensity) { .... command_stream.str().c_str(); .... } ``` Что это такое? Побочный эффект рефакторинга? Недописанный код? Безобидная лишняя строчка или ошибка? Есть два места, где можно увидеть таких одиноких странников: * V530 The return value of function 'c\_str' is required to be utilized\. ZeissCAN\.cpp 1553 * V530 The return value of function 'c\_str' is required to be utilized\. ZeissCAN\.cpp 2800 ## "Брамины" ![0242_Travel_to_Microcos_ru/image7.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image7.png) ```cpp int LeicaScopeInterface::GetDICTurretInfo(....) { .... std::string tmp; .... if (tmp == "DIC-TURRET") scopeModel_->dicTurret_.SetMotorized(true); else scopeModel_->dicTurret_.SetMotorized(true); .... } ``` Вот так выглядит программный "[брамин](https://fallout.fandom.com/ru/wiki/%d0%91%d1%80%d0%b0%d0%bc%d0%b8%d0%bd)"\. Независимо, выполнится условие или нет, выполняется один и тот же код\. Предупреждение: V523 The 'then' statement is equivalent to the 'else' statement\. LeicaDMIScopeInterface\.cpp 1296 Ещё одна ошибка схожего рода\. Здесь сравниваются одинаковые строки\. Наверное, этот код содержит опечатку: ```cpp 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) .... } ``` Предупреждение: V549 The first argument of 'strcmp' function is equal to the second argument\. XLedDev\.cpp 119 ## Что\-то не стыкуется ![0242_Travel_to_Microcos_ru/image8.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image8.png) Значения 'false' и 'true' могут неявно приводиться к типу 'int': * false превращается в 0; * true превращается в 1\. Например, вот такой код успешно скомпилируется: ```cpp int F() { return false; } ``` Функция F\(\) возвращает 0\. Иногда люди ошибаются и вместо статуса ошибки, который имеет тип 'int', возвращают 'false' или 'true'\. Происходит это по забывчивости\. Ничего страшного, если статус ошибки кодируется значением 0\. Беда возникает в том случае, если статусы ошибок кодируются значениями, отличными от нуля\. Именно это происходит в проекте μManager\. Имеются следующие предопределённые значения: ```cpp #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 .... ``` Обратите внимание, что 0 означает, что всё хорошо\. Другие значения сообщают о наличии какой\-то ошибки\. Мне кажется, в коде проекта μManager имеется путаница со статусами и значениями 'true', 'false'\. Рассмотрим функцию CreateProperty\(\): ```cpp int MM::PropertyCollection::CreateProperty(....) { if (Find(pszName)) return DEVICE_DUPLICATE_PROPERTY; .... if (!pProp->Set(pszValue)) return false; .... return DEVICE_OK; } ``` Обратите внимание, что если вызов pProp\-\>Set\(pszValue\) закончился неудачно, то функция возвращает 'false'\. Получается, что функция возвращает статус DEVICE\_OK\. Это очень странно\. Другой подозрительный фрагмент кода: ```cpp 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; } ``` В конце мы видим "return true;"\. Это означает, что функция вернёт статус DEVICE\_ERR 1 \(generic, undefined error\)\. При этом, мне кажется, что на самом деле всё хорошо\. Возможно читать покажется странным, почему я называю эти места подозрительными, а не говорю, что это ошибки\. Дело в том, что местами 'false' используется специально, чтобы выделить особые случаи\. Пример: ```cpp 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 } .... } ``` Обратите внимание на комментарий\. Произошла ошибка\. Но мы притворимся, что всё хорошо, вернув ноль\. Возможно, 'false' написан вместо DEVICE\_OK, чтобы подчеркнуть особенность этого кода\. Таких комментариев правда только парочка\. А вот в остальных местах непонятно, ошибка это или "хитрый финт ушами"\. Рискну предположить, что в половине мест всё правильно, а половина действительно окажутся ошибками\. ![0242_Travel_to_Microcos_ru/image9.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image9.png) В любом случае такой код очень плохо пахнет\. Вот список всех подозрительных мест: * V601 The 'false' value is 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 implicitly casted to the integer type\. PIGCSControllerCom\.cpp 405 * V601 The 'false' value is implicitly casted to the integer type\. Prior\.cpp 778 * V601 The 'false' value is implicitly casted to the integer type\. Prior\.cpp 2308 * V601 The 'false' value is implicitly casted to the integer type\. Prior\.cpp 2313 * V601 The 'false' value is implicitly casted to the integer type\. Prior\.cpp 2322 * V601 The 'false' value is implicitly casted to the integer type\. SutterLambda\.cpp 190 * V601 The 'false' value is implicitly casted to the integer type\. SutterLambda\.cpp 269 * V601 The 'false' value is implicitly casted to the integer type\. SutterLambda\.cpp 285 * V601 The 'false' value is implicitly casted to the integer type\. Tofra\.cpp 900 * V601 The 'false' value is implicitly casted to the integer type\. Tofra\.cpp 1806 * V601 The 'false' value is implicitly casted to the integer type\. Tofra\.cpp 1830 ## Странный Get ![0242_Travel_to_Microcos_ru/image10.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image10.png) ```cpp int pgFocus::GetOffset(double& offset) { MM_THREAD_GUARD_LOCK(&mutex); deviceInfo_.offset = offset; MM_THREAD_GUARD_UNLOCK(&mutex); return DEVICE_OK; } ``` Мне кажется, или с эти кодом что\-то не в порядке? Анализатору этот код не нравится: 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 И действительно, странно\. Функция называется "Get\_\_\_\_"\. Функция возвращает статус\. А ещё она принимает аргумент 'offset' по ссылке\. И\.\.\. И не записывает ничего в него\. Я не знаю, как всё это работает\. Но, быть может, надо было сделать присваивание наоборот? Как\-то так: ```cpp offset = deviceInfo_.offset; ``` Ещё одна подозрительная функция GetTransmission\(\): ```cpp int SpectralLMM5Interface::GetTransmission(...., double& transmission) { .... int16_t tr = 0; memcpy(&tr, answer + 1, 2); tr = ntohs(tr); transmission = tr/10; .... } ``` Предупреждение PVS\-Studio: 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 Обратите внимание, что возвращаемое значение имеет тип double \(речь идёт о transmission\)\. Но вычисляется это значение странно\. Целочисленное значение делится на 10\. У меня есть сильное подозрение, что происходит потеря точности\. Например, если 'tr' равно 5, то после деления мы получим 0, а вовсе не 0\.5\. Наверное, правильный код должен выглядеть так: ```cpp transmission = tr/10.0; ``` ## Ошибка или не ошибка? Первое впечатление может быть обманчиво ![0242_Travel_to_Microcos_ru/image11.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image11.png) В языке Си/Си\+\+, числа начинающиеся с нуля считаются заданными в восьмеричном формате\. В проекте μManager есть одно подозрительное место: ```cpp 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: V536 Be advised that the utilized constant value is represented by an octal form\. Oct: 010, Dec: 8\. LeicaDMSTCHub\.cpp 142 Не понятно, действительно хотели использовать число 8, написанное в восьмеричном формате или это ошибка\. В других местах в функцию SetCommand\(\) передаются числа, записанные в десятичной системе счисления\. Например, так: ```cpp int ret = SetCommand(device, core, xyStage_, 35, ack); ``` Не знаю, найдена ошибка или нет, но упомянуть про это место стоит\. ### Перфекционист негодует ![0242_Travel_to_Microcos_ru/image12.png](https://import.viva64.com/docx/blog/0242_Travel_to_Microcos_ru/image12.png) Есть масса мелочей, которые не являются существенными\. Однако, почти все программисты перфекционисты\. Давайте поворчим\. Полно лишних строчек\. Один из примеров: ```cpp 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; ..... } ``` Вторая проверка явно лишняя\. Другой пример: ```cpp 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; .... } ``` Вторая проверка опять не имеет смысла\. Перед ней переменная 'ret' нигде не изменится\. Вторую проверку можно смело удалить\. Таких лишних проверок достаточно много\. Приведу их списком: [Micro\-Manager\-V571\-V649\.txt](https://pvs-studio.com/external-pictures/txt/micro-manager-v571-v649.txt)\. Ещё из мелочи можно отметить неправильной формат при работе с функциями sprintf\(\)\. Беззнаковая переменная распечатывается, как знаковая\. Это может привести к некорректной распечатке больших значений\. ```cpp int MP285Ctrl::Initialize() { .... unsigned int nUm2UStepUnit = MP285::Instance()->GetUm2UStep(); .... sprintf(sUm2UStepUnit, "%d", nUm2UStepUnit); .... } ``` Нашлось три таких места: * V576 Incorrect format\. Consider checking the third actual argument of the 'sprintf' function\. The SIGNED integer type argument is expected\. MP285Ctrl\.cpp 253 * V576 Incorrect format\. Consider checking the third actual argument of the 'sprintf' function\. The SIGNED integer type argument is expected\. MP285Ctrl\.cpp 276 * V576 Incorrect format\. Consider checking the third actual argument of the 'sprintf' function\. The SIGNED integer type argument is expected\. MP285Ctrl\.cpp 327 ## Заключение Единичная проверка этого и любого другого проекта малоэффективна\. Пользу можно получить только при регулярном использовании статических анализаторов кода\. Тогда многие ошибки и опечатки будут исправлены на самом раннем этапе\. Рассматривайте статический анализ, как расширение предупреждений, которые выдаёт компилятор\. Командам, создающим средние и крупные проекты для операционной системы Windows, мы рекомендуем использовать наш статический анализатор [PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/0222/)\. Цена зависит от размера команды и требуемого уровня поддержки\. Тем, кто работает с Linux, мы предлагаем обратить внимание на бесплатный анализатор кода [Cppcheck](https://pvs-studio.ru/ru/blog/terms/0083/)\. Или попробовать [Standalone](https://pvs-studio.ru/ru/blog/posts/0219/) версию PVS\-Studio\.