Единорог заинтересовался микромиром

Андрей Карпов
Статей: 371



В этот раз интересные примеры ошибок нам преподнёс микромир. Мы проверили с помощью анализатора кода PVS-Studio открытый проект μManager. Это программный пакет для автоматизированного получения изображения с микроскопа.

Picture 1

μManager

Это относительно небольшой проект. Объем исходного кода около 11 мегабайт. Для чего именно нужен этот проект, я не знаю. Меня попросили его проверить. И вот единорог уже спешит на помощь. Наверное, нужный и полезный проект, раз попросили.

Сайт проекта: Micro-Manager.

Анализ как всегда выполнен с помощью анализатора PVS-Studio. Кстати, если вы пропустили, то вот сравнение, которое так долго ждали наши потенциальные пользователи: "Сравнение анализаторов кода: CppCat, Cppcheck, PVS-Studio, Visual Studio".

На этом лирическое отступление окончено. Начнём рассматривать интересные фрагменты кода.

long != int

Picture 24

Проект μManager претендует на кроссплатформенность. Поэтому, надо быть аккуратным с типом 'long'. В 32-битных системах размер типа 'long' совпадает с размером типа 'int'. А вот в 64-битных системах может быть по-разному. В Win64 тип 'long' остался 32-битным. В 64-битном мире Linux принята другая модель данных, в которой 'long' является 64-битным. Нужно проявлять бдительность, используя этот тип.

Проект μManager содержит следующий неудачный код:

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

Останови уплотнитель мусора!

Picture 2
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', которое превращается в единицу.

Условие должно быть таким:

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

Идентичные сравнения

Picture 23
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

Есть ещё один фрагмент кода с аналогичной ошибкой. Видимо, с установкой позиции какого-то колёсика будут проблемы:

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: 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

Кажется, мы о чём-то забыли

Picture 3

Предлагаю посмотреть вот на этот код. Заметите, чего в нём не хватает?

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

Одинокой странник

Picture 10
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

"Брамины"

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

Вот так выглядит программный "брамин". Независимо, выполнится условие или нет, выполняется один и тот же код.

Предупреждение: V523 The 'then' statement is equivalent to the 'else' statement. LeicaDMIScopeInterface.cpp 1296

Ещё одна ошибка схожего рода. Здесь сравниваются одинаковые строки. Наверное, этот код содержит опечатку:

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

Что-то не стыкуется

Picture 6

Значения 'false' и 'true' могут неявно приводиться к типу 'int':

  • false превращается в 0;
  • true превращается в 1.

Например, вот такой код успешно скомпилируется:

int F() { return false; }

Функция F() возвращает 0.

Иногда люди ошибаются и вместо статуса ошибки, который имеет тип 'int', возвращают 'false' или 'true'. Происходит это по забывчивости. Ничего страшного, если статус ошибки кодируется значением 0.

Беда возникает в том случае, если статусы ошибок кодируются значениями, отличными от нуля. Именно это происходит в проекте μManager.

Имеются следующие предопределённые значения:

#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():

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. Это очень странно.

Другой подозрительный фрагмент кода:

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' используется специально, чтобы выделить особые случаи. Пример:

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, чтобы подчеркнуть особенность этого кода.

Таких комментариев правда только парочка. А вот в остальных местах непонятно, ошибка это или "хитрый финт ушами". Рискну предположить, что в половине мест всё правильно, а половина действительно окажутся ошибками.

Picture 11

В любом случае такой код очень плохо пахнет.

Вот список всех подозрительных мест:

  • 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

Picture 7
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' по ссылке. И... И не записывает ничего в него. Я не знаю, как всё это работает. Но, быть может, надо было сделать присваивание наоборот? Как-то так:

offset = deviceInfo_.offset;

Ещё одна подозрительная функция GetTransmission():

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.

Наверное, правильный код должен выглядеть так:

transmission = tr/10.0;

Ошибка или не ошибка? Первое впечатление может быть обманчиво

Picture 9

В языке Си/Си++, числа начинающиеся с нуля считаются заданными в восьмеричном формате. В проекте μ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: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 010, Dec: 8. LeicaDMSTCHub.cpp 142

Не понятно, действительно хотели использовать число 8, написанное в восьмеричном формате или это ошибка. В других местах в функцию SetCommand() передаются числа, записанные в десятичной системе счисления. Например, так:

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

Не знаю, найдена ошибка или нет, но упомянуть про это место стоит.

Перфекционист негодует

Picture 8

Есть масса мелочей, которые не являются существенными. Однако, почти все программисты перфекционисты. Давайте поворчим.

Полно лишних строчек. Один из примеров:

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

Вторая проверка явно лишняя.

Другой пример:

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.

Ещё из мелочи можно отметить неправильной формат при работе с функциями sprintf(). Беззнаковая переменная распечатывается, как знаковая. Это может привести к некорректной распечатке больших значений.

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. Цена зависит от размера команды и требуемого уровня поддержки.

Тем, кто работает с Linux, мы предлагаем обратить внимание на бесплатный анализатор кода Cppcheck. Или попробовать Standalone версию PVS-Studio.



Найдите ошибки в своем C, C++, C# и Java коде

Предлагаем попробовать проверить код вашего проекта с помощью анализатора кода PVS-Studio. Одна найденная в нём ошибка скажет вам о пользе методологии статического анализа кода больше, чем десяток статей.

goto PVS-Studio;

Андрей Карпов
Статей: 371


Найденные ошибки

Проверено проектов
344
Собрано ошибок
12 970

А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++, C#
и Java

goto PVS-Studio;