We have checked the header files from the Embarcadero C++Builder XE3 project. In fact, it means that we have checked just a small number of inline-functions. Accordingly, quite few issues were found, but they are enough to write a small post.
We regularly check open-source projects and many other things that can be checked. For instance, we once checked the libraries included into Visual C++ 2012, which resulted in publishing the post "Errors detected in the Visual C++ 2012 libraries".
The Visual C++ distribution kit includes the libraries' source codes. But the things are worse with C++Builder: there are only header files available, so we managed to analyze only some of the inline-functions. However, we did find some interesting issues. Let's see what these are.
#pragma warning(disable : 4115)
#include <objbase.h>
#pragma warning(default : 4115)PVS-Studio's diagnostic message:
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: 16, 18. iaguid.h 18
It's no good setting the warning output mode to the default state. A good practice is to save and then restore the previous state. Use "#pragma warning(push[ ,n ])" and "#pragma warning(pop)" to do that.
#define SET_VTYPE_AND_VARREF(type, val) \
this->vt = VT_ ## type | VT_BYREF; \
V_ ## type ## REF (this) = val;
TVariantT& operator=(System::Currency* src)
{
Clear();
if(src)
SET_VTYPE_AND_VARREF(CY,
reinterpret_cast<tagCY*>(&(src->Val)));
return* this;
}PVS-Studio's diagnostic message:
V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. utilcls.h 1781
The SET_VTYPE_AND_VARREF macro is bad. Its contents are not framed with curly brackets { }. This results in the "if (src)" condition referring only to the first line of the macro.
#define _BITS_BYTE 8
template<class _Uint,
_Uint _Ax,
_Uint _Cx,
_Uint _Mx>
class linear_congruential
{
static _CONST_DATA int _Nw =
(_BITS_BYTE * sizeof (_Uint) + 31) / 32;
void seed(seed_seq& _Seq)
{
_Uint _Arr[3 + _Nw];
....
int _Lsh = _BITS_BYTE * sizeof (_Uint);
....
for (int _Idx = _Nw; 0 < --_Idx; )
_Arr[3 + _Idx - 1] |=
_Arr[3 + _Idx] << _Lsh;
....
}
}PVS-Studio's diagnostic message:
V610 Instantiate linear_congruential < unsigned long, 40014, 0, 2147483563 >: Undefined behavior. Check the shift operator '<<. The right operand '_Lsh' is greater than or equal to the length in bits of the promoted left operand. random 738
The '_Lsh' variable takes value 32 in this function. You can't shift 32-bit types more than by 31 bits. Here's a quote from the standard specification: The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand.
The DXVABitMask macro is implemented in a dangerous way too:
#define DXVABitMask(__n) (~((~0) << __n))Here is another quote from the standard specification on this: Otherwise, if E1 has a signed type and non-negative value, and E1*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.
Because of this macro PVS-Studio generates several warnings. For example:
V610 Undefined behavior. Check the shift operator '<<. The left operand '(~0)' is negative. dxva.h 1080
To learn more about shifts and undefined behavior see the post: Wade not in unknown waters. Part three.
The code appeared to contain a lot of fragments where, after calling the 'new' operator, pointers are checked for not being NULL pointers. It is pointless now and even harmful: if a memory allocation error occurs, the 'new' operator throws the exception std::bad_alloc.
We can call the 'new' operator so that it won't throw exceptions. C++ Builder even has a special macro for that purpose:
#define NEW_NOTHROW(_bytes) new (nothrow) BYTE[_bytes]But there are some fragments where the memory allocation issue wasn't solved. For example:
inline void _bstr_t::Assign(BSTR s) throw(_com_error)
{
if (m_Data != NULL) {
m_Data->Assign(s);
}
else {
m_Data = new Data_t(s, TRUE);
if (m_Data == NULL) {
_com_issue_error(E_OUTOFMEMORY);
}
}
}PVS-Studio's diagnostic message:
V668 There is no sense in testing the 'm_Data' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. comutil.h 454
The line "_com_issue_error(E_OUTOFMEMORY);" is never executed. If an error occurs, the exception std::bad_alloc() will be thrown.
static inline BYTE *__CorHlprNewThrows(size_t bytes)
{
BYTE *pbMemory = new BYTE[bytes];
if (pbMemory == NULL)
__CorHlprThrowOOM();
return pbMemory;
}PVS-Studio's diagnostic message:
V668 There is no sense in testing the 'pbMemory' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. corhlpr.h 56
template<class TYPE, class ARG_TYPE>
void CDXArray<TYPE, ARG_TYPE>::SetSize(int nNewSize, int nGrowBy)
{
....
TYPE* pNewData = (TYPE*) new BYTE[nNewMax * sizeof(TYPE)];
// oh well, it's better than crashing
if (pNewData == NULL)
return;
....
}PVS-Studio's diagnostic message:
V668 There is no sense in testing the 'pNewData' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. dxtmpl.h 338
All the rest code fragments are much alike, and there is no point citing them. Let me only give you the list of diagnostic messages:
All these bugs were found only in inline-functions! I can imagine what horrible stuff can be found in the *.cpp files. :)
At the moment when I finished writing this article, Embarcadero C++Builder XE4 was released. Nevertheless, this fact doesn't diminish the value of the analysis we have performed, as it has demonstrated PVS-Studio's capabilities very well.
Thank you all for your attention. I hope that the C++Builder developers will take notice of our post and get interested in checking the compiler and libraries' source files. In conclusion, I want to share a few useful links with you: