Одним из популярных файловых менеджеров в среде Microsoft Windows является Far Manager, принявший эстафету у Norton Commander, созданной еще для DOS. Far Manager позволяет облегчить работу с файловой системой (создание, редактирование, просмотр, копирование, перемещение, поиск, удаление файлов), а также расширяет стандартный функционал (работа с сетью, архивами, резервными копиями и т.д.). Недавно была произведена работа по портированию Far Manager на Linux, и на текущий момент была выпущена альфа-версия. Команда PVS-Studio не могла обойти стороной данное событие и решила проверить качество адаптированного кода.

Немного о Far Manager

Far Manager — консольный файловый менеджер для операционных систем семейства Microsoft Windows, ориентированный на работу с клавиатурой. FAR Manager наследует двухоконную идеологию, стандартную (по умолчанию) расцветку и систему команд (управление с клавиатуры) у известного файлового менеджера Norton Commander и предоставляет удобный интерфейс пользователя для работы с файлами (создания файлов и каталогов, просмотра, редактирования, копирования, переименования, удаления и т.д.).

Рисунок 1 — Far Manager 2 на Windows (нажмите на картинку для увеличения)

Автор программы — Евгений Рошал. Первая версия была выпущена 10 сентября 1996 года. Последняя версия, к которой приложил руку Рошал, датируется 23 июня 2000 года (версия 1.65). Фактически с того момента разработкой FAR Manager начала заниматься группа «FAR Group». Следующий релиз v1.70 датируется уже 29 марта 2006 года. 13 декабря 2008 года выходит версия 2.0, программа стала бесплатной (open source) и распространяется под модифицированной лицензией BSD. Все версии Far от 1.70 до 2.0 практически не имеют внешних отличий, и не требуют от пользователя каких-либо дополнительных усилий по освоению программы при переходе на новую версию. Начиная с версии 1.80 появилась поддержка Unicode. Последней выпущенной версией считается 3.0 от 4 ноября 2016 года.

10 августа 2016 года была опубликована первая тестовая сборка файлового менеджера Far2l (Linux). На данный момент сборка содержит встроенный работающий терминал, а также плагины Align, AutoWrap, Colorer, DrawLine, Editcase, FarFTP, FarLng, MultiArc, NetBox, SimpleIndent, TmpPanel. Код распространяется под лицензией GPLv2.

Рисунок 2 — Far Manager 2 на Linux (нажмите на картинку для увеличения)

От слов к делу

После проверки проекта Far2l было получено 1038 предупреждений общего назначения. На следующей диаграмме представлено распределение предупреждений по уровням достоверности:

Рисунок 1 — Распределение предупреждений по уровням достоверности (важности)

Кратко прокомментируем приведенную диаграмму: было получено 153 предупреждения уровня High, 336 предупреждений уровня Medium, 549 предупреждений уровня Low.

Несмотря на достаточно большое число предупреждений, стоит помнить, что не каждое из них является реальной ошибкой. Просмотрев отчет, содержащий только предупреждения уровня High и Medium, мной было выделено 250 случаев, являющихся скорее всего реальными ошибками.

Если брать уровни High и Medium, то получается, что процент ложных срабатываний составил около 49%. Другими словами, каждое второе предупреждение выявляет дефект в коде.

Теперь определим относительную плотность реальных ошибок, найденных анализатором PVS-Studio. Суммарное количество строк исходного кода (SLOC) — 538675. Следовательно, плотность будет равна 0.464 ошибки на 1000 строк кода. Когда-нибудь мы соберем эти числа и напишем обобщающую статью о качестве кода различных проектов.

Стоит отметить, что данный показатель не демонстрирует общую плотность ошибок по всему проекту — она может быть, как больше (анализатор не сработал на реальной ошибке), так и меньше (анализатор сработал на правильном коде). Как правило, на других проектах мы получаем большую плотность найденных ошибок. Можно сказать, что с точки зрения качества кода портирование прошло успешно. Впрочем, настоятельно рекомендуется исправить найденные ошибки, ведь они далеко небезобидные.

Результаты проверки

Хочу заранее предупредить, что для удобства чтения код будет подвергаться рефакторингу. Также, в статье содержатся далеко не все ошибки, обнаруженные в ходе проверки, а только наиболее интересные из них.

Copy-Paste

Предупреждение анализатора: V501 There are identical sub-expressions 'Key == MCODE_F_BM_GET' to the left and to the right of the '||' operator. macro.cpp 4819

int KeyMacro::GetKey()
{
  ....
  DWORD Key = !MR ? MCODE_OP_EXIT : GetOpCode(MR, Work.ExecLIBPos++);
  ....
  switch (Key)
  {
  ....
  case MCODE_F_BM_POP:
  {
    TVar p1, p2;

    if (Key == MCODE_F_BM_GET)
      VMStack.Pop(p2);

    if (   Key == MCODE_F_BM_GET    // <=
        || Key == MCODE_F_BM_DEL 
        || Key == MCODE_F_BM_GET    // <=
        || Key == MCODE_F_BM_GOTO)
    {
      VMStack.Pop(p1);
    }
    ....
  }
  }
}

Переменную Key дважды сравнили с константой MCODE_F_BM_GET. Вероятно, это опечатка, и Key следовало сравнить ещё c какой-то константой. Анализатор нашел еще 3 похожих места:

• V501 There are identical sub-expressions '!StrCmpN(CurStr, L"!/", 2)' to the left and to the right of the '||' operator. fnparce.cpp 291
• V501 There are identical sub-expressions '!StrCmpN(CurStr, L"!=/", 3)' to the left and to the right of the '||' operator. fnparce.cpp 291
• V501 There are identical sub-expressions 'KEY_RCTRL' to the left and to the right of the '|' operator. keyboard.cpp 1830

Предупреждение анализатора: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 267, 268. APIStringMap.cpp 268

static BOOL WINPORT(GetStringType)( DWORD type,
                                    LPCWSTR src,
                                    INT count,
                                    LPWORD chartype )
{
  ....
  while (count--)
  {
    int c = *src;
    WORD type1, type3 = 0; /* C3_NOTAPPLICABLE */
    ....
    if ((c>=0xFFE0)&&(c<=0xFFE6)) type3 |= C3_FULLWIDTH; // <=
    if ((c>=0xFFE0)&&(c<=0xFFE6)) type3 |= C3_SYMBOL;    // <=
    ....
  }
  ....
}

Видимо, второе условие писалось по принципу Copy-Paste и оно абсолютно идентично первому. Однако, если замысел в этом и заключался, то можно упростить код, убрав второе условие:

....
if ((c>=0xFFE0)&&(c<=0xFFE6)) type3 |= C3_FULLWIDTH | C3_SYMBOL; 
....

Найденная ошибка оказалась далеко не единственной:

• V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 272, 273. APIStringMap.cpp 273
• V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 274, 275. APIStringMap.cpp 275
• V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 6498, 6503. macro.cpp 6503
• V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 1800, 1810. vmenu.cpp 1810
• V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 3353, 3355. wrap.cpp:3355

Предупреждение анализатора: V523 The 'then' statement is equivalent to the 'else' statement. Queque.cpp 358

void FTP::AddToQueque(FAR_FIND_DATA* FileName, 
                      LPCSTR Path, 
                      BOOL Download)
{
  ....
  char *m;
  ....
  if(Download)
    m = strrchr(FileName->cFileName, '/'); // <=
  else
    m = strrchr(FileName->cFileName, '/'); // <=
  ....
}

Подозреваю, что и здесь условие писалось по принципу «Copy-Paste»: вне зависимости от значения Download (TRUE, FALSE), в указатель m будет сохранена позиция последнего вхождения символа '/'.

Неопределенное поведение

Предупреждение анализатора: V567 Undefined behavior. The 'Item[FocusPos]->Selected' variable is modified while being used twice between sequence points. dialog.cpp 3827

int Dialog::Do_ProcessSpace()
{
  ....
  if (Item[FocusPos]->Flags & DIF_3STATE)
    (++Item[FocusPos]->Selected) %= 3;       // <=
  else
    Item[FocusPos]->Selected = !Item[FocusPos]->Selected;
  ....
}

Здесь налицо явное неопределенное поведение: переменную Item[FocusPos]->Selected дважды меняют в одной точке следования (инкремент и деление по модулю 3 с присвоением результата).

Было найдено еще одно место с подобным неопределенным поведением:

• V567 Undefined behavior. The '::ViewerID' variable is modified while being used twice between sequence points. viewer.cpp 117

Предупреждение анализатора: V610 Undefined behavior. Check the shift operator '<<'. The right operand 'sizeof (wchar_t) * 8' is greater than or equal to the length in bits of the promoted left operand. RegExp.cpp 4467

#define rechar wchar_t
#define RE_CHAR_COUNT (1 << sizeof(rechar) * 8)

int RegExp::Optimize()
{
  ....
  for (op=code; ; op=op->next)
  {
    switch (OP.op)
    {
    ....
    case opType:
    {
      for (int i = 0; i < RE_CHAR_COUNT; i++)    // <=
      {
        if (ISTYPE(i, OP.type))
        {
          first[i]=1;
        }
      }
      
      break;
    }
    }
    ....
  }
  ....
}

Суть ошибки заключается в следующем: в Linux тип wchar_t имеет размер 4 байта. Следовательно, происходит сдвиг знакового int (4 байта) на 32 бита влево. Согласно стандарту C++11, если левый операнд является знаковым положительным числом, сдвиг влево на N бит приведет к неопределенному поведению, если N не меньше, чем количество бит в левом операнде. Корректный код будет выглядеть следующим образом:

#define rechar wchar_t
#define RE_CHAR_COUNT (static_cast<int64_t>(1) << sizeof(rechar) * 8)

int RegExp::Optimize()
{
  ....
  for (int64_t i = 0; i < RE_CHAR_COUNT; i++)
  {
    ....
  }
  ....
}

Были найдены еще несколько мест, ведущие к неопределенному поведению при сдвиге влево:

• V610 Undefined behavior. Check the shift operator '<<'. The right operand 'sizeof (wchar_t) * 8' is greater than or equal to the length in bits of the promoted left operand. RegExp.cpp 4473
• V610 Undefined behavior. Check the shift operator '<<'. The right operand 'sizeof (wchar_t) * 8' is greater than or equal to the length in bits of the promoted left operand. RegExp.cpp 4490
• V610 Undefined behavior. Check the shift operator '<<'. The right operand 'sizeof (wchar_t) * 8' is greater than or equal to the length in bits of the promoted left operand. RegExp.cpp 4537
• V610 Undefined behavior. Check the shift operator '<<'. The right operand 'sizeof (wchar_t) * 8' is greater than or equal to the length in bits of the promoted left operand. RegExp.cpp 4549
• V610 Undefined behavior. Check the shift operator '<<'. The right operand 'sizeof (wchar_t) * 8' is greater than or equal to the length in bits of the promoted left operand. RegExp.cpp 4561

Неверная работа с памятью

Начнем новый раздел с небольшой разминки. Предлагаю попытаться найти ошибку самостоятельно (подсказка — она в функции TreeItem::SetTitle).

class UnicodeString
{
  ....
  UnicodeString(const wchar_t *lpwszData) 
  { 
    SetEUS(); 
    Copy(lpwszData); 
  }
  ....
  const wchar_t *CPtr() const { return m_pData->GetData(); }
  operator const wchar_t *() const { return m_pData->GetData(); }
  ....
}

typedef UnicodeString FARString;

struct TreeItem
{
  FARString strName;
  ....
}

TreeItem **ListData;

void TreeList::SetTitle()
{
  ....
  if (GetFocus())
  {
    FARString strTitleDir(L"{");
    const wchar_t *Ptr = ListData 
                         ? ListData[CurFile]->strName
                         : L""; 
    ....
  }
  ....
}

Предупреждение анализатора: V623 Consider inspecting the '?:' operator. A temporary object of the 'UnicodeString' type is being created and subsequently destroyed. Check third operand. treelist.cpp 2093

Неочевидная ошибка, не правда ли? В текущем контексте переменная ListData[CurFile]->strName является объектом класса UnicodeString. В классе UnicodeString перегружен оператор неявного преобразования в тип const wchar_t*. Теперь обратите внимание на тернарный оператор в функции TreeList::SetTitle: второй и третий операнды являются разными типами (UnicodeString и const char[1] соответственно). Подразумевалось, что если первый операнд вернет false, то указатель Ptr будет адресоваться на пустую строку. Поскольку конструктор класса UnicodeString не объявлен как explicit, на самом деле будет создан временный объект, содержащий пустую строку, который неявно приведется к типу const wchar_t*; далее временный объект уничтожится и Ptr будет указывать на невалидные данные. Исправленный код будет выглядеть таким образом:

....
const wchar_t *Ptr = ListData 
                     ? ListData[CurFile]->strName.CPtr()
                     : L"";
....

Следующий код примечателен тем, что на него сработали одновременно две диагностики.

Предупреждения анализатора:

• V779 Unreachable code detected. It is possible that an error is present. 7z.cpp 203
• V773 The function was exited without releasing the 't' pointer. A memory leak is possible. 7z.cpp 202

BOOL WINAPI _export SEVENZ_OpenArchive(const char *Name,
                                       int *Type)
{
  Traverser *t = new Traverser(Name);
  if (!t->Valid()) 
  {
    return FALSE;
    delete t;
  }
  
  delete s_selected_traverser;
  s_selected_traverser = t;
  return TRUE;
}

Что же здесь можно обнаружить? Во-первых, действительно, в операторе if имеется недостижимый код: если условие выполняется, то функция возвращает FALSE, завершая свою работу. А из-за недостижимого кода всего-навсего произошла утечка памяти — объект по указателю t не удаляется. Чтобы исправить ошибки, достаточно поменять два оператора местами внутри блока.

Следующий код покажет, как можно ошибиться при определении размера объекта класса (структуры) через указатель.

Предупреждения анализатора:

• V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'PInfo' class object. filelist.cpp 672
• V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'PInfo' class object. filelist.cpp 673

int64_t FileList::VMProcess(int OpCode,
                            void *vParam,
                            int64_t iParam)
{
  switch (OpCode)
  {
  ....
  case MCODE_V_PPANEL_PREFIX:           // PPanel.Prefix
  {
    PluginInfo *PInfo = (PluginInfo *)vParam;
    memset(PInfo, 0, sizeof(PInfo));          // <=
    PInfo->StructSize = sizeof(PInfo);        // <=
    ....
  }
  ....
  }
}

Обе ошибки заключаются в том, что sizeof(PInfo) вместо ожидаемого размера структуры вернет размер указателя (4 или 8 байт). Соответственно, memset заполнит нулями только первые 4 (8) байт структуры, а также в поле PInfo->StructSize запишется размер указателя. Корректный код будет выглядеть следующим образом:

....
PluginInfo *PInfo = (PluginInfo*)vParam;
memset(PInfo, 0, sizeof(*PInfo));
PInfo->StructSize = sizeof(*PInfo);
....

Анализатор обнаружил еще пару похожих мест:

• V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'HistoryItem' class object. history.cpp 594
• V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'handle' class object. plugins.cpp 682

Странные условия

И снова предлагаю небольшую разминку — попробуйте самостоятельно найти ошибку в следующей части кода:

int FTP::ProcessKey(int Key, unsigned int ControlState)
{
  ....
  if(   !ShowHosts 
     && (ControlState == 0 || ControlState == PKF_SHIFT) 
     && Key == VK_F6)
  {
    FTP *ftp = OtherPlugin(this);
    int  rc;

    if(   !ftp 
       && ControlState == 0 
       && Key == VK_F6)
    {
      return FALSE;
    }
    ....
  }
  ....
}

Предупреждение анализатора: V560 A part of conditional expression is always true: Key == 0x75. Key.cpp 493

Обратите внимание на внешнее и внутреннее условия: в них Key сравнивается с константой VK_F6. Если поток управления достигает внутреннего условия, то переменная Key гарантировано будет равна VK_F6 и вторая проверка этой переменной будет лишней. В упрощённом виде второе условие будет выглядеть так:

....
if(   !ftp 
   && ControlState == 0)
{
  return FALSE;
}
....

Анализатор предупреждает об этой ошибке и о некоторых подобных:

• V560 A part of conditional expression is always true: !cps. DString.cpp 47
• V560 A part of conditional expression is always true: !ShowHosts. FGet.cpp 139
• V560 A part of conditional expression is always false: !wsz. cnDownload.cpp 190
• V560 A part of conditional expression is always true: !UserReject. extract.cpp 485
• And 8 additional diagnostic messages.

Предупреждение анализатора: V503 This is a nonsensical comparison: pointer <= 0. fstd_exSCPY.cpp 8

char *WINAPI StrCpy(char *dest, LPCSTR src, int dest_sz)
{
  if(dest <= 0)   // <=
    return NULL;
  ....
}

Данный код содержит бессмысленное сравнение указателя с отрицательным числом (указатели не работают с областями памяти с отрицательными адресами). Исправленный код может выглядеть следующим образом:

....
if(dest == nullptr)
  return NULL;
....

Предупреждение анализатора: V584 The FADC_ALLDISKS value is present on both sides of the '==' operator. The expression is incorrect or it can be simplified. findfile.cpp 3116

enum FINDASKDLGCOMBO
{
  FADC_ALLDISKS,
  FADC_ALLBUTNET,
  ....
};

FindFiles::FindFiles()
{
  
  ....
  if (   FADC_ALLDISKS + SearchMode == FADC_ALLDISKS     // <=
      || FADC_ALLDISKS + SearchMode == FADC_ALLBUTNET)
  {
    ....
  }
  ....
}

Анализатор обнаружил подозрительное первое подусловие. Исходя из перечисления FINDASKDLGCOMBO, константа FADC_ALLDISKS равна 0, FADC_ALLBUTNET равна 1. Если подставить численные значения констант в условные выражение, то получим следующее:

if (   0 + SearchMode == 0
    || 0 + SearchMode == 1)
{
  ....
}

Исходя из кода выше, все условие можно упростить:

if (   SearchMode == FADC_ALLDISKS
    || SearchMode == FADC_ALLBUTNET)
{
  ....
}

Некорректная работа с форматной строкой

Предупреждение анализатора: V576 Incorrect format. Consider checking the fourth actual argument of the 'swprintf' function. The char type argument is expected. FarEditor.cpp 827

void FarEditor::showOutliner(Outliner *outliner)
{
  ....
  wchar_t cls = 
    Character::toLowerCase((*region)[region->indexOf(':') + 1]);
  
  si += swprintf(menuItem+si, 255-si, L"%c ", cls); // <=
  ....
}

Вероятно, это ошибка портирования. Проблема заключается в том, что в Visual C++ в функциях вывода широких строк нестандартно интерпретируются спецификаторы в форматной строке: %c ожидает широкий символ (wide char, wchar_t). В Linux дела обстоят иначе: в соответствии со стандартом по спецификатору %c будет ожидаться многобайтовый символ (multibyte symbol, char). Корректный код будет выглядеть следующим образом:

si += swprintf(menuItem+si, 255-si, L"%lc ", cls);

Предупреждение анализатора: V576 Incorrect format. Consider checking the fourth actual argument of the 'swprintf' function. The pointer to string of char type symbols is expected. cmddata.cpp 257

void CommandData::ReadConfig()
{
  ....
  wchar Cmd[16];
  ....
  wchar SwName[16+ASIZE(Cmd)];
  swprintf(SwName,ASIZE(SwName), L"switches_%s=", Cmd);  // <=
  ....
}

Похожая ситуация: форматная строка содержит спецификатор %s, следовательно, ожидается многобайтовая строка (char*). Однако, следующим параметром была передана широкая строка (wchar_t*). Корректный код будет выглядеть следующим образом:

swprintf(SwName,ASIZE(SwName), L"switches_%ls=", Cmd);

Анализатор также предупреждает и о двух других неверных способах передачи параметров в соответствии с форматной строкой:

• V576 Incorrect format. Consider checking the third actual argument of the 'fprintf' function. The char type argument is expected. vtansi.cpp 1033
• V576 Incorrect format. Consider checking the third actual argument of the 'fprintf' function. The char type argument is expected. vtansi.cpp 1038

Выводы

Что можно сказать про порт Far на Linux? Да, ошибок было найдено достаточно, однако не стоит забывать, что проект находится в состоянии альфа-версии и продолжает развиваться. Применяя методологию статического анализа кода, ошибки можно найти еще на раннем этапе и не допустить их в репозиторий. Стоит отметить, что все преимущества статического анализа будут проявляться только при его регулярном использовании (в крайнем случае — во время «ночных» сборок).

От своего лица я предлагаю оценить пользу статического анализа с помощью PVS-Studio: продукт доступен для Microsoft Windows и deb/rpm-based дистрибутивов Linux, позволяя быстро и регулярно проверять Ваш проект. Также, если Вы студент, индивидуальный разработчик, или участвуете в разработке открытого некоммерческого проекта, то предлагается возможность бесплатного использования PVS-Studio.

В этом ознакомительном видео Вы можете узнать, как установить PVS-Studio для Linux и быстро проверить свой проект (на примере Far Manager):



Также, если Вы знаете интересный проект, который следовало бы проверить, то Вы можете предложить его нам на GitHub. Подробнее в статье: "Предложи проект для проверки анализатором PVS-Studio: теперь и на GitHub".

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Phillip Khandeliants. Porting is a Delicate Matter: Checking Far Manager under Linux