Dolphin Smalltalk 7 is dedicated to Open Sourse

The other day, ObjectArts fully opened the source code and released the language and development environment of Dolphin Smalltalk under an open source MIT license! I could not pass by without trying to check this project with the help of the PVS-Studio code analyzer. I can congratulate the developers that they managed to create a high quality code. I could not find meaningful errors. However, as always, there are a number of bugs and smelling code. I hope thanks to this article the code will be a little better.

about the project

Dolphin Smalltalk is a development environment in its own Smalltalk dialect for Windows. The key features are tight integration with native widgets and operating system subsystems, including COM and ActiveX, and nice looking graphic design.

For a long time, Dolphin Smalltalk was available in two versions: shareware limited edition (community edition) and a paid package for professional development. The latter gave access to all functions, including advanced editors and the publication of applications in standalone mode, but cost about four hundred dollars.
')
Using PVS-Studio 6.00, the open source Dolphin Smalltalk Virtual Machine has been verified. The following are the results of testing with a static analyzer. Despite the fact that the DolphinVM project is very small, there are still suspicious places in its code.

Test results

Warning N1: V611 : The memory was allocated using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] msg;'. compiler.cpp 379

Compiler::StaticType Compiler::FindNameAsStatic(....) { .... char* msg = new char[strlen(szPrompt)+name.size()+32]; ::wsprintf(msg, szPrompt, name.c_str()); char szCaption[256]; ::LoadString(GetResLibHandle(), IDR_COMPILER, szCaption, ....); int answer = ::MessageBox(NULL, msg, szCaption, ....); delete msg; //<==?? .... } 

The analyzer detected an error related to the fact that memory is allocated and released in incompatible ways.

After calling the operator “new []”, it is necessary to free the memory using the operator “delete []”.

Warning N2 : V716 Suspicious type conversion in return statement: returned BOOL, but function actually returns HRESULT. idolphinstart.cpp 78

 #define STDMETHODIMP HRESULT STDMETHODCALLTYPE STDMETHODIMP CDolphinSmalltalk::GetVersionInfo(LPVOID pvi) { extern BOOL __stdcall GetVersionInfo(VS_FIXEDFILEINFO* ....); return ::GetVersionInfo(static_cast<VS_FIXEDFILEINFO*>(pvi)); } 

In this code snippet, the “BOOL” type is implicitly cast to the “HRESULT” type. While such an operation is quite acceptable from the point of view of the C ++ language, it has no practical meaning. The HRESULT type is intended for storing status, has a rather complicated format and has nothing to do with the BOOL type.

Warning N3 : V701 realloc () possible leak: when realloc () fails in allocating memory, original pointer 'elems' is lost. Consider assigning realloc () to a temporary pointer. compiler.cpp 2922

 POTE Compiler::ParseByteArray() { NextToken(); while (m_ok && !ThisTokenIsClosing()) { if (elemcount>=maxelemcount) { _ASSERTE(maxelemcount > 0); maxelemcount *= 2; elems = (BYTE*)realloc(elems, maxelemcount*sizeof(BYTE)); } .... } .... } 

This code is potentially dangerous: it is recommended that the result of the realloc () function be stored in another variable. The realloc () function changes the size of a block of memory. If it is not possible to change the size of the memory block at the moment, the function will return a null pointer. The main problem is that when using a construction like "ptr = realloc (ptr, ...)" the ptr pointer to this data block may be lost.

Similar dangerous places:

• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'm_pAllocations' is lost. Consider assigning realloc () to a temporary pointer. alloc.cpp 436
• V701 realloc () possible leak: when realloc () fails in allocating memory, the original pointer 'pUnmarked' is lost. Consider assigning realloc () to a temporary pointer. gc.cpp 217

Warning N4 : V547 Expression 'i> = 0' is always true. Unsigned type value is always> = 0. compact.cpp 35

 // Answer the index of the last occuppied OT entry unsigned __stdcall ObjectMemory::lastOTEntry() { HARDASSERT(m_pOT); // HARDASSERT(m_nInCritSection > 0); unsigned i = m_nOTSize-1; const OTE* pOT = m_pOT; while (pOT[i].isFree()) { ASSERT(i >= 0); i--; } return i; } 

Most likely there is no error, but the code is suspicious. In turn, the elements of the array are iterated until the isFree () function for one of the elements returns false. Wrong here is ASSERT. He doesn't actually check anything. The variable 'i' is unsigned, which means it is always greater than or equal to 0.

Another comparison '> = 0' with unsigned type:

• V547 Expression is always true. Unsigned type value is always> = 0. loadimage.cpp 343

Warning N5 : V730 class are initialized inside the constructor. Consider inspecting: m_dwSize. imagefilemapping.h 13

 class ImageFileMapping { HANDLE m_hFile; HANDLE m_hMapping; LPVOID m_pData; DWORD m_dwSize; public: ImageFileMapping() : m_hFile(0), m_hMapping(0), m_pData(NULL){} ~ImageFileMapping() { Close(); } .... }; 

Another case of potentially dangerous code. The “ImageFileMapping” class contains only 4 fields, but in the constructor only three of them are assigned an initial value. The 'm_dwSize' member remains uninitialized.

This is a fairly common practice when the class does not work with "size" if the pointer to the array is still zero. However, it is very easy to make a mistake, so it is better to initialize all members of the class.

Similar classes:

• V730 class are initialized inside the constructor. Consider inspecting: m_flags, m_oopWorkspacePools, m_context, m_compiledMethodClass. compiler.cpp 84
• V730 class are initialized inside the constructor. Consider inspecting: m_tokenType, m_integer, tp, m_cc, m_base. lexer.cpp 40

Warning N6 : V665 Possibly (default: X) 'is incorrect in this context. The '#pragma warning (push / pop)' should be used instead. Check lines: 99, 101. compact.cpp 101

 // Perform a compacting GC size_t ObjectMemory::compact() { .... #pragma warning (disable : 4127) while(true) #pragma warning (default : 4127) .... } 

Programmers often believe that after the “pragma warning (default: X)” directive, the warnings will again take effect, previously disabled using the “pragma warning (disable: X)”. This is not true. The 'pragma warning (default: X)' directive sets the warning with the number 'X' to the state that is valid DEFAULT. This is not the same thing.

The correct code is:

 size_t ObjectMemory::compact() { .... #pragma warning(push) #pragma warning (disable : 4127) while(true) #pragma warning(pop) .... } 

A good article on this topic is: " So you want to silence this warning in Visual C ++ ."

The entire list of such places:

• V665 Possibly, the usage of #pragma warning (default: X) 'is incorrect in this context. The '#pragma warning (push / pop)' should be used instead. Check lines: 244, 246. expire.cpp 246
• V665 Possibly, the usage of #pragma warning (default: X) 'is incorrect in this context. The '#pragma warning (push / pop)' should be used instead. Check lines: 226, 241. expire.cpp 241
• V665 Possibly, the usage of #pragma warning (default: X) 'is incorrect in this context. The '#pragma warning (push / pop)' should be used instead. Check lines: 126, 128. interfac.cpp 128
• V665 Possibly, the usage of #pragma warning (default: X) 'is incorrect in this context. The '#pragma warning (push / pop)' should be used instead. Check lines: 385, 387. interprt.cpp 387

Warning N7 : V576 Incorrect format. Consider checking the fourth argument of the wsprintfA function. To print the value of the% p 'should be used. interfac.cpp 679

 inline DWORD __stdcall Interpreter::GenericCallbackMain(SMALLINTEGER id, BYTE* lpArgs) { .... #ifdef _DEBUG { char buf[128]; wsprintf(buf, "WARNING: .... (%d, %x)\n", id, lpArgs); WarningWithStackTrace(buf); } #endif .... } 

Very often, the pointer value is attempted to be printed using the '% x' specifier.

This code is erroneous, since it will work only on those systems where the pointer size coincides with the size of the 'int' type. And, for example, in Win64, this code already prints only the lower part of the 'ptr' pointer. In this case, you must use the '% p' ​​specifier.

Warning N8 : V547 Expression 'ch> 127' is always false. The value range of char type: [-128, 127]. decode.cpp 55

 ostream& operator<<(ostream& stream, const VariantCharOTE* oteChars) { .... char ch = string->m_characters[i]; //if (ch = '\0') break; if (ch < 32 || ch > 127) //<== { static char hexChars[16+1] = "0123456789ABCDEF"; .... } .... } 

By default, the 'char' type has a range of [-127; 127]. Using the compilation flag / J, you can tell the compiler to use the range [0; 255]. But to compile this source file, such a flag is not specified, so checking “ch> 127” does not make sense.

Warning N9 : V688 The 'prev' function argument. thrdcall.h 126

 void LinkAfter(T* prev) { T* pThis = static_cast<T*>(this); this->next = prev->next; if (this->next) this->next->prev = pThis; this->prev = prev; prev->next = pThis; } 

Most likely, there is no error in this function, but to call the same name the parameters of the functions of the class and the members of the class is not a very good style of writing code. This could potentially lead to a typo due to which the value of the wrong variable will be used or changed.

Warning N10 : V601 The 'false' value is cast to the integer type. compiler.cpp 1940

 int Compiler::ParseUnaryContinuation(...., int textPosition) { int continuationPointer = m_codePointer; MaybePatchLiteralMessage(); while (m_ok && (ThisToken()==NameConst)) { int specialCase=false; //<== .... if (!specialCase) //<== { int sendIP = GenMessage(ThisTokenText(), 0, textPosition); AddTextMap(sendIP, textPosition, ThisTokenRange().m_stop); } .... } .... } 

In this case, this warning is a recommendation. If the 'specialCase' variable works everywhere as a logical variable, then it is better to use the standard 'bool' type for this.

Conclusion

Another project went to the list of open projects verified by us.

Preparing such articles, we provide in them information far from all warnings that the static analyzer gives. Therefore, we recommend that the authors of the project independently perform an analysis and study all the messages issued by the analyzer.

And as always I remind the reader that the value of analyzers is not in single checks, but in regular use.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Dolphin Smalltalk 7's Source Code .