Geekly Articles each Day
📜 ⬆️ ⬇️

Porting is a delicate matter: checking Far Manager for Linux

One of the popular file managers in the Microsoft Windows environment is Far Manager, who took the baton from Norton Commander, which was created for DOS. Far Manager makes it easy to work with the file system (creating, editing, viewing, copying, moving, searching, deleting files), and also expands the standard functionality (work with the network, archives, backup copies, etc.). Recently, work has been done on porting Far Manager to Linux, and an alpha version has been released to date. The PVS-Studio team could not ignore this event and decided to check the quality of the adapted code.
Picture 24

A bit about Far Manager


Far Manager is a console file manager for operating systems of the Microsoft Windows family, focused on working with a keyboard. FAR Manager inherits a two-window ideology, standard (default) colors and command system (keyboard control) from the well-known Norton Commander file manager and provides a convenient user interface for working with files (creating files and directories, viewing, editing, copying, renaming, deleting etc.).

Figure 1 - Far Manager 2 on Windows (click on image to enlarge)


Figure 1 - Far Manager 2 on Windows (click on image to enlarge)

The author of the program is Eugene Roshal. The first version was released on September 10, 1996. The latest version, to which Roshal had a hand, is dated June 23, 2000 (version 1.65). In fact, since the development of the FAR Manager, the FAR Group has been involved. The next release v1.70 is dated March 29, 2006. On December 13, 2008, version 2.0 is released, the program has become free (open source) and distributed under a modified BSD license. All Far versions from 1.70 to 2.0 have virtually no external differences, and do not require any additional efforts from the user to master the program when switching to the new version. Since version 1.80, Unicode support has been added. The latest released version is 3.0 dated November 4, 2016.
')
On August 10, 2016, the first test build of the Far2l file manager (Linux) was published. At the moment, the assembly contains a built-in working terminal, as well as Align, AutoWrap, Colorer, DrawLine, Editcase, FarFTP, FarLng, MultiArc, NetBox, SimpleIndent, TmpPanel plugins. The code is distributed under the GPLv2 license.

Figure 2 - Far Manager 2 on Linux (click on image to enlarge)


Figure 2 - Far Manager 2 on Linux (click on image to enlarge)

From words to deeds


After checking the project Far2l, 1038 general-purpose warnings were received. The following diagram shows the distribution of warnings by confidence level:

Figure 1 - Distribution of warnings by confidence (importance)


Figure 1 - Distribution of warnings by confidence (importance)

We briefly comment on the diagram: 153 warnings of the High level, 336 warnings of the Medium level, 549 warnings of the Low level were received.

Despite the rather large number of warnings, it is worth remembering that not every one of them is a real mistake. After reviewing the report containing only warnings of the High and Medium level, I highlighted 250 cases, which are most likely real errors.

If we take the levels High and Medium, it turns out that the percentage of false positives was about 49%. In other words, every second warning reveals a defect in the code.

Now let's determine the relative density of real errors found by the PVS-Studio analyzer. The total number of lines of the source code (SLOC) is 538675. Therefore, the density will be equal to 0.464 errors per 1000 lines of code. Someday we will collect these numbers and write a general article about the quality of the code of various projects.

It should be noted that this indicator does not demonstrate the overall density of errors for the entire project - it can be more (the analyzer did not work on a real error) or less (the analyzer worked on the correct code). As a rule, on other projects we get the greater density of errors found. We can say that from the point of view of code quality, the porting was successful. However, it is strongly recommended to correct the errors found, because they are far from harmless.

Test results


I want to warn you in advance that for readability, the code will be refactored. Also, the article contains not all errors found during the test, but only the most interesting ones.

Copy paste


Picture 28


Analyzer Warning : V501 There are identical sub-expressions 'Key == MCODE_F_BM_GET' || 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); } .... } } }

The Key variable was twice compared with the constant MCODE_F_BM_GET . This is probably a typo, and Key should be compared with some constant. The analyzer found 3 more similar places:


Analyzer Warning : V581 The conditional expressions of the 'if' are agreed 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; // <= .... } .... }

Apparently, the second condition was written on the principle of Copy-Paste and it is absolutely identical to the first. However, if this was the intention, then the code can be simplified by removing the second condition:

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

The error found was far from the only one:


Analyzer Warning : 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, '/'); // <= .... }

I suspect that here the condition was written on the “Copy-Paste” principle: regardless of the Download value ( TRUE , FALSE ), the position of the last occurrence of the '/' character will be saved in the m pointer.

Indefinite behavior


Picture 37


Analyzer Warning : V567 Undefined behavior. The 'Item [FocusPos] -> Selected' variable is used while being used 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; .... }

Here there is a clear undefined behavior: the variable Item [FocusPos] -> Selected is changed twice in one sequence point (increment and modulo 3 division with assignment of the result).

Another place with similar undefined behavior was found:


Analyzer warning : V610 Undefined behavior. Check the shift operator '<<'. The size of the operand (wchar_t) * 8 'is the 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; } } .... } .... }

The essence of the error is as follows: in Linux, the wchar_t type is 4 bytes in size. Consequently, a sign int (4 bytes) is shifted 32 bits to the left. According to the C ++ 11 standard, if the left operand is a significant positive number, shifting left by N bits will result in undefined behavior if N is not less than the number of bits in the left operand. The correct code will look like this:

 #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++) { .... } .... }

Several more locations were found leading to unspecified behavior when shifting to the left:


Wrong work with memory


Picture 41



Let's start a new section with a little warm-up. I suggest trying to find the error yourself (the hint is in the TreeItem :: SetTitle function ).

 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""; .... } .... }

Analyzer warning : V623 Consider inspecting the '?:' Operator. A temporary object of the UnicodeString type is being created and subsequently destroyed. Check third operand. treelist.cpp 2093

An obvious mistake, isn't it? In the current context, the ListData variable [CurFile] -> strName is an object of the UnicodeString class. In the UnicodeString class, the implicit conversion operator is overloaded to the const wchar_t * type . Now notice the ternary operator in the TreeList :: SetTitle function : the second and third operands are of different types ( UnicodeString and const char [1], respectively). The implication was that if the first operand returns false , then the Ptr pointer will be addressed to an empty string. Since the constructor of the UnicodeString class is not declared as explicit , it will actually create a temporary object containing an empty string that will implicitly cast to const wchar_t * ; then the temporary object will be destroyed and Ptr will indicate invalid data. The corrected code will look like this:

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

The following code is notable for the fact that two diagnostics worked on it simultaneously.

Analyzer Warnings :


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

What can be found here? First, indeed, there is an unreachable code in the if statement : if the condition is met, the function returns FALSE , completing its work. And because of the unreachable code, only a memory leak occurred - the object by pointer t is not deleted. To correct errors, it is enough to swap two operators inside the block.

The following code will show how you can make a mistake when determining the size of an object of a class (structure) through a pointer.

Analyzer Warnings :


 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); // <= .... } .... } }

Both errors are that sizeof (PInfo) instead of the expected structure size will return a pointer size (4 or 8 bytes). Accordingly, memset will fill only the first 4 (8) bytes of the structure with zeros, and the pointer size will be written in the PInfo-> StructSize field. The correct code will look like this:

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

The analyzer found a couple of similar places:


Strange conditions


Picture 39


And again I suggest a small warm-up - try to find the error yourself in the next part of the code:

 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; } .... } .... }

Analyzer Warning : V560 A part of conditional expression is always true: Key == 0x75. Key.cpp 493

Pay attention to the external and internal conditions: in them, Key is compared with the constant VK_F6 . If the control flow reaches an internal condition, then the Key variable is guaranteed to be equal to VK_F6 and the second check of this variable will be superfluous. In simplified form, the second condition will look like this:

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

The analyzer warns about this error and some similar ones:


Analyzer Warning : 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; .... }

This code contains a meaningless comparison of a pointer with a negative number (pointers do not work with memory areas with negative addresses). The revised code might look like this:

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

Analyzer Warning : V584 The FADC_ALLDISKS value is present on both sides of the '==' operator. The expression is not correct. findfile.cpp 3116

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

The analyzer detected a suspicious first sub-condition. Based on the FINDASKDLGCOMBO enumeration, the FADC_ALLDISKS constant is 0, FADC_ALLBUTNET is 1. If we substitute the numerical values ​​of the constants in the conditional expression, we get the following:

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

Based on the code above, the whole condition can be simplified:

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

Incorrect work with the format string


Picture 40


Analyzer warning : V576 Incorrect format. Consider checking the fourth 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); // <= .... }

This is probably a porting error. The problem is that in Visual C ++, wide-string output functions interpret specifiers in a format string non-routinely: % c expects a wide character (wide char, wchar_t ). In Linux, things are different: in accordance with the standard specified by the % c specifier, a multibyte symbol (multibyte symbol, char ) will be expected. The correct code will look like this:

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

Analyzer warning : V576 Incorrect format. Consider checking the fourth argument of the swprintf function. The type of 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); // <= .... }

A similar situation: the format string contains the % s specifier, therefore, a multibyte string ( char * ) is expected. However, the following parameter was passed a wide string ( wchar_t * ). The correct code will look like this:

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

The analyzer also warns of two other incorrect ways of passing parameters in accordance with the format string:


findings


What can I say about the Far port on Linux? Yes, mistakes were found enough, but do not forget that the project is in the alpha version and continues to evolve. Using the methodology of static code analysis, errors can be found at an early stage and prevent them from entering the repository. It is worth noting that all the benefits of static analysis will manifest themselves only with its regular use (as a last resort - during the "night" assemblies).

On my own behalf, I propose to evaluate the benefits of static analysis using PVS-Studio: the product is available for Microsoft Windows and deb / rpm-based Linux distributions, allowing you to quickly and regularly check your project. Also, if you are a student, an individual developer, or participate in the development of an open non-commercial project, then the possibility of using PVS-Studio for free is offered.

In this introductory video you can learn how to install PVS-Studio for Linux and quickly check your project (using the example of Far Manager):



Also, if you know an interesting project that you should check out, then you can offer it to us on GitHub . Read more in the article: " Offer a project for testing with the PVS-Studio analyzer: now on GitHub ".



If you want to share this article with an English-speaking audience, then please use the link to the translation: Phillip Khandeliants. Porting is a Delicate Matter: Checking Far Manager under Linux

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/321948/

More articles:


All Articles