Verifying an open WinSCP project being developed in Embarcadero C ++ Builder

We constantly check open C / C ++ projects. But almost always, these are projects developed in Visual Studio. But we somehow deprived Embarcadero C ++ Builder with attention. It needs to be corrected and today we checked the WinSCP project.

WinSCP

WinSCP is a free graphical client of SFTP and SCP protocols designed for Windows. Distributed under the GNU GPL. Provides secure copying of files between computers and servers supporting these protocols.

Official website: http://winscp.net

Embarcadero C ++ Builder XE2 is required to build the project.
')

Check

The verification was carried out with the help of the PVS-Studio analyzer. At the moment, PVS-Studio supports:

• Visual Studio 2013 C, C ++, C ++ 11, C ++ / CX (WinRT)
• Visual Studio 2012 C, C ++, C ++ 11, C ++ / CX (WinRT)
• Visual Studio 2010 C, C ++, C ++ 0x
• Visual Studio 2008 C, C ++
• Visual Studio 2005 C, C ++
• Embarcadero RAD Studio XE5 C, C ++, C ++ 11, 64-bit compiler included
• Embarcadero RAD Studio XE4 C, C ++, C ++ 11, 64-bit compiler included
• Embarcadero RAD Studio XE3 Update 1 C, C ++, C ++ 11, 64-bit compiler included
• Embarcadero RAD Studio XE2 C, C ++, C ++ 0x
• Embarcadero RAD Studio XE C, C ++
• Embarcadero RAD Studio 2010 C, C ++
• Embarcadero RAD Studio 2009 C, C ++
• MinGW C, C ++, C ++ 11

Plus you can run PVS-Studio Standalone. It allows you to check pre-prepared * .i files or track the project build process and collect all the information you need to check. Details can be found here: " PVS-Studio now supports any build system ."

Test results

There were not many errors, but enough to write this article and attract the attention of users of Embarcadero RAD Studio.

The arguments to the memset () function are confused

TForm * __fastcall TMessageForm::Create(....) { .... LOGFONT AFont; .... memset(&AFont, sizeof(AFont), 0); .... } 

PVS-Studio warning: V575 The 'memset' function processes '0' elements. Inspect the third argument. messagedlg.cpp 786

The memset () function takes the size of the array in the third argument. Simple but nasty typo. The structure remains uninitialized.

There is another identical typo in the code below: messagedlg.cpp 796

Using a non-existent object

 void __fastcall TCustomScpExplorerForm::EditorAutoConfig() { .... else { .... TEditorList EditorList; EditorList = *WinConfiguration->EditorList; EditorList.Insert(0, new TEditorPreferences(EditorData)); WinConfiguration->EditorList = &EditorList; } .... } 

PVS-Studio warning: V506 Pointer to local variable; EditorList; Such a pointer will become invalid. customscpexplorer.cpp 2633

The 'EditorList' object will be destroyed immediately after going out of scope. However, the program stores a pointer to this object and then uses it. This leads to undefined behavior.

Garbage in the dialogue

 bool __fastcall RecursiveDeleteFile(....) { SHFILEOPSTRUCT Data; memset(&Data, 0, sizeof(Data)); .... Data.pTo = L""; .... } 

PVS-Studio warning: V540 Member 'pTo' should point be terminated by two 0 characters. common.cpp 1659

Note the following line in the description of the pTo parameter in MSDN: "This string must be double-null terminated."

Due to an error in the dialogue to work with the file will contain garbage. Or will not. It all depends on luck. But the code is wrong anyway.

Duplicated string

 int CFileZillaApi::Init(....) { .... m_pMainThread->m_hOwnerWnd=m_hOwnerWnd; m_pMainThread->m_hOwnerWnd=m_hOwnerWnd; .... } 

PVS-Studio warning: V519 The m_pMainThread-> m_hOwnerWnd 'variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 88, 89. filezillaapi.cpp 89

Perhaps there is no mistake. Just one extra line.

Idle check

 STDMETHODIMP CShellExtClassFactory::CreateInstance(....) { .... CShellExt* ShellExt = new CShellExt(); if (NULL == ShellExt) { return E_OUTOFMEMORY; } .... } 

PVS-Studio warning: V668 It was no use. The exception will be generated in the case of memory allocation error. dragext.cpp 554

The “if (NULL == ShellExt)” check does not make sense, since if the memory cannot be allocated, the 'new' operator will throw an exception std :: bad_alloc.

Dangerous way to use the fprintf () function

 bool CAsyncSslSocketLayer::CreateSslCertificate(....) { .... char buffer[1001]; int len; while ((len = pBIO_read(bio, buffer, 1000)) > 0) { buffer[len] = 0; fprintf(file, buffer); } .... } 

V618 It is dangerous to call the specification. The example of the safe code: printf ("% s", str); asyncsslsocketlayer.cpp 2247

If, when writing to a file, the buffer contains control specifiers, the result will be unpredictable. Safe way:

 fprintf(file, "%s", buffer); 

In general, this error can be considered a potential vulnerability .

Something is wrong with the variable 'err'

 static error_t client_send_propfind_request(....) { .... error_t err = 0; int code = 0; apr_hash_t * props = NULL; const char * target = path_uri_encode(remote_path, pool); char * url_path = apr_pstrdup(pool, target); WEBDAV_ERR(neon_get_props(&props, ras, url_path, NEON_DEPTH_ZERO, starting_props, false, pool)); if (err && (err == WEBDAV_ERR_DAV_REQUEST_FAILED)) .... } 

Warning: V560 A part of conditional expression is always false: (err == 1003). webdavfilesystem.cpp 10990

Conclusion

Where are you, programmers using Embarcadero RAD Studio? Hey! As our statistics show, there are almost none. Come and try the PVS-Studio code analyzer!

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Check out the WinSCP Developed in Embarcadero C ++ Builder .