Портирование — дело тонкое: проверка Far Manager под Linux

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 строк кода. Когда-нибудь мы соберем эти числа и напишем обобщающую статью о качестве кода различных проектов.

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

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

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

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

Переменную 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

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

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

• 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

Подозреваю, что и здесь условие писалось по принципу «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

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

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

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

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

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

• 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).

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

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

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

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

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

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

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

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

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

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

• 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

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

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

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

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

• 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.

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

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

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

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

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

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

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

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

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

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