Портування — справа тонка: перевірка Far Manager під Linux

Одним з популярних файлових менеджерів в середовищі Microsoft Windows, Far Manager, прийняв естафету у Norton Commander, створеної ще для DOS. Far Manager дозволяє полегшити роботу з файловою системою (створення, редагування, перегляд, копіювання, переміщення, пошук, видалення файлів), а також розширює стандартний функціонал (робота з мережею, архівами, резервними копіями тощо). Нещодавно була проведена робота по портированию Far Manager на Linux, і на поточний момент була випущена альфа-версія. Команда PVS-Studio не могла обійти стороною цю подію і вирішила перевірити якість адаптованого коду.
<img src=«habrastorage.org/getpro/habr/post_images/653/5ce/b44/6535ceb440001a72c4d871694d28f6c9.png» alt=«Picture » 24" />

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

Рисунок 1 - Far Manager 2 на Windows (натисніть на картинку для збільшення)
Малюнок 1 — Far Manager 2 на Windows (натисніть на картинку для збільшення)

Автор програми — Євген Рошал. Перша версія була випущена 10 вересня 1996 року. Остання версія, до якої доклав руку Рошал, датується 23 червня 2000 року (версія 1.65). Фактично з того моменту розробкою FAR Manager почала займатися група «FAR Group». Наступний реліз v1.70 датується вже 29 березня 2006 року. 13 грудня 2008 року виходить версія 2.0, програма стала безкоштовною (freeware) і поширюється під модифікованою ліцензією 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 (натисніть на картинку для збільшення)
Малюнок 2 — Far Manager 2 на Linux (натисніть на картинку для збільшення)

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

Рисунок 1 - Розподіл попереджень за рівнями достовірності (важливості)
Малюнок 1 — Розподіл попереджень за рівнями достовірності (важливості)

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

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

Якщо брати рівні High і Medium, то виходить, що відсоток помилкових спрацьовувань склав близько 49%. Іншими словами, кожне друге попередження виявляє дефект в коді.

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

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

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

Copy-Paste
<img src=«habrastorage.org/getpro/habr/post_images/83a/8e4/e4a/83a8e4e4a6285c34e50dd7abc9cdf4a2.png» alt=«Picture » 28" />

Попередження аналізатора: 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 слід порівняти ще з якийсь константою. Аналізатор знайшов ще 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»: незалежно від значення Завантажити (TRUE, FALSE), покажчик m буде збережена позиція останнього входження символу '/'.

Невизначений поведінка
<img src=«habrastorage.org/getpro/habr/post_images/119/ef8/92e/119ef892e282a2048af006f6302d452a.png» alt=«Picture » 37" />

Попередження аналізатора: 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
Невірна робота з пам'яттю
Picture 41

Почнемо новий розділ з невеликої розминки. Пропоную спробувати знайти помилку самостійно (підказка — вона у функції 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
Дивні умови
Picture 39

І знову пропоную невелику розминку — спробуйте самостійно знайти помилку в наступній частині коду:
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)
{
....
}

Некоректна робота з форматного рядка
Picture 40

Попередження аналізатора: 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);
<b> </b>

Попередження аналізатора: 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

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.
Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.