В этот раз интересные примеры ошибок нам преподнёс микромир. Мы проверили с помощью анализатора кода PVS-Studio открытый проект μManager. Это программный пакет для автоматизированного получения изображения с микроскопа.
Это относительно небольшой проект. Объем исходного кода около 11 мегабайт. Для чего именно нужен этот проект, я не знаю. Меня попросили его проверить. И вот единорог уже спешит на помощь. Наверное, нужный и полезный проект, раз попросили.
Сайт проекта: Micro-Manager.
Анализ как всегда выполнен с помощью анализатора PVS-Studio. Кстати, если вы пропустили, то вот сравнение, которое так долго ждали наши потенциальные пользователи: "Сравнение анализаторов кода: CppCat, Cppcheck, PVS-Studio, Visual Studio".
На этом лирическое отступление окончено. Начнём рассматривать интересные фрагменты кода.
Проект μ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:
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();
....
}Что это такое? Побочный эффект рефакторинга? Недописанный код? Безобидная лишняя строчка или ошибка?
Есть два места, где можно увидеть таких одиноких странников:
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':
Например, вот такой код успешно скомпилируется:
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, чтобы подчеркнуть особенность этого кода.
Таких комментариев правда только парочка. А вот в остальных местах непонятно, ошибка это или "хитрый финт ушами". Рискну предположить, что в половине мест всё правильно, а половина действительно окажутся ошибками.
В любом случае такой код очень плохо пахнет.
Вот список всех подозрительных мест:
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);
....
}Нашлось три таких места:
Единичная проверка этого и любого другого проекта малоэффективна. Пользу можно получить только при регулярном использовании статических анализаторов кода. Тогда многие ошибки и опечатки будут исправлены на самом раннем этапе. Рассматривайте статический анализ, как расширение предупреждений, которые выдаёт компилятор.
Командам, создающим средние и крупные проекты для операционной системы Windows, мы рекомендуем использовать наш статический анализатор PVS-Studio. Цена зависит от размера команды и требуемого уровня поддержки.
Тем, кто работает с Linux, мы предлагаем обратить внимание на бесплатный анализатор кода Cppcheck. Или попробовать Standalone версию PVS-Studio.