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

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

μManager

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

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

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

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

long != int

Проект μ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

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

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

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

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

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

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

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

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

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

«Брамины»

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

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

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

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

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

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

• 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

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;

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

В языке Си/Си++, числа начинающиеся с нуля считаются заданными в восьмеричном формате. В проекте μ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);

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

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

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

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

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

Для небольших команд и индивидуальных разработчиков мы предлагаем инструмент CppCat. Индивидуальная лицензия — $250. Продление — $200. При покупке нескольких лицензий — скидки.

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

P.S.

Перевод этой статьи: "The Unicorn's Travel to the Microcosm".

Автор: Andrey2008

Источник