Обнаружены ошибки в библиотеках C++Builder

Мы проверили заголовочные файлы, входящие в состав Embarcadero C++Builder XE3. Фактически, это означает только проверку небольшого числа inline-функций. Соответственно найдено совсем немного подозрительных мест, но достаточно для небольшой заметки.

Введение

Мы регулярно проверяем открытые проекты, а так же все другое, что можно проверить. Например, мы проверяли библиотеки, входящие в состав Visual C++ 2012. Результатом стала заметка: "Обнаружены ошибки в библиотеках Visual C++ 2012".

В дистрибутив Visual C++ входят исходные коды библиотек. Ситуация с C++Builder хуже. Есть только заголовочные файлы. Поэтому удалось проверить только некоторые inline-функции. Тем не менее, удалось найти кое-что интересное. Рассмотрим соответствующие участки кода.

Работа с предупреждениями

#pragma warning(disable : 4115) #include <objbase.h> #pragma warning(default : 4115)

Диагностическое сообщение, выданное PVS-Studio:

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

Нехорошо устанавливать режим вывода предупреждения в состояние по умолчанию. Правильным подходом, будет сохранить, а затем восстановить предыдущее состояние. Для это следует использовать "#pragma warning(push[ ,n ])" и "#pragma warning(pop)".

Неудачный макрос

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

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

Макрос SET_VTYPE_AND_VARREF написан плохо. Его содержимое не обрамлено фигурными скобками { }. В результате условие «if (src)» будет относиться только к первой строчке макроса.

Неопределенное поведение

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

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

В этой функции, переменная ‘_Lsh’ примет значение 32. Нельзя сдвигать 32-битные типы, более чем на 31 бит. Выдержка из стандарта: 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.

Также, опасным образом сделан макрос DXVABitMask:

#define DXVABitMask(__n) (~((~0) << __n))

Процитируем соответствующую строчку из стандарта: 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.

Из-за этого макроса, PVS-Studio выдает несколько предупреждений. Пример:

V610 Undefined behavior. Check the shift operator ‘<<. The left operand ‘(~0)’ is negative. dxva.h 1080

Подробнее о сдвигах и неопределенном поведении можно почитать в заметке: Не зная брода, не лезь в воду. Часть третья.

Неучтенное изменение в поведении оператора new.

Обнаружилось достаточно много мест, где после вызова оператора ‘new’, проверяется, что указатель не равен NULL. Сейчас это не имеет смысла и даже вредно. В случае ошибки выделения памяти, оператор ‘new’ генерирует исключение std::bad_alloc.

Можно вызывать оператор ‘new’, который не генерирует исключение. В C++Builder даже есть специальный макрос для этого:

#define NEW_NOTHROW(_bytes) new (nothrow) BYTE[_bytes]

Однако видимо не везде удалось навести порядок с выделением памяти. Приведу несколько примеров:

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:

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

Строчка "_com_issue_error(E_OUTOFMEMORY);" никогда не выполняется. В случае ошибки будет сгенерировано исключение std::bad_alloc().

static inline BYTE *__CorHlprNewThrows(size_t bytes) {   BYTE *pbMemory = new BYTE[bytes];   if (pbMemory == NULL)     __CorHlprThrowOOM();   return pbMemory; }

Диагностическое сообщение, выданное PVS-Studio:

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:

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

Остальные фрагменты кода похожи, и приводить их смысла нет. Ограничусь только диагностическими сообщениями:

• V668 There is no sense in testing the ‘p’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. d3dx10math.inl 1008
• V668 There is no sense in testing the ‘p’ 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 123
• 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 395
• V668 There is no sense in testing the ‘m_pHashTable’ 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 1126
• V668 There is no sense in testing the ‘newBrush’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusbrush.h 44
• V668 There is no sense in testing the ‘retimage’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusbrush.h 374
• V668 There is no sense in testing the ‘argbs’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusbrush.h 615
• V668 There is no sense in testing the ‘argbs’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusbrush.h 645
• V668 There is no sense in testing the ‘argbs’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdipluspath.h 1196
• V668 There is no sense in testing the ‘argbs’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdipluspath.h 1231
• V668 There is no sense in testing the ‘argbs’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdipluspath.h 1372
• V668 There is no sense in testing the ‘argbs’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdipluspath.h 1405
• V668 There is no sense in testing the ‘newLineCap’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdipluslinecaps.h 153
• V668 There is no sense in testing the ‘nativeRegions’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusgraphics.h 1415
• V668 There is no sense in testing the ‘newRegion’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusregion.h 89
• V668 There is no sense in testing the ‘nativeFamilyList’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusfontcollection.h 57
• V668 There is no sense in testing the ‘newImage’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusbitmap.h 334
• V668 There is no sense in testing the ‘bitmap’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusbitmap.h 819
• V668 There is no sense in testing the ‘bitmap’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. gdiplusbitmap.h 862
• V668 There is no sense in testing the ‘m_pData’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. spcollec.h 266
• 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. spcollec.h 325

И это только в inline-функциях! Представляю, что творится в *.cpp файлах. 🙂

Примечание

В тот момент, когда я закончил писать эту статью, вышел Embarcadero C++Builder XE4. Впрочем, это не отменяет пользу от проделанного анализа и хорошо демонстрирует возможности PVS-Studio. Плюс я ещё немножко подождал с публикацией этой статьи. Сегодня мы выпустили новую версию PVS-Studio, которая уже поддерживает C++Builder XE4. Это хороший повод для публикации и предложения попробовать наш анализатор кода.

Заключение

Спасибо всем за внимание. Надеюсь, разработчики C++Builder обратят на нас внимание и захотят проверить с помощью PVS-Studio исходные коды компилятора и библиотек. В заключении, хочу предложить вниманию несколько полезных ссылок:

1. Описание инструмента PVS-Studio. Можно скачать полнофункциональную пробную версию.
2. Андрей Карпов. C++Builder, сборка 64-битных приложений и ренессанс Viva64.
3. Наш твиттер @Code_Analysis. Публикуем много интересных ссылок по тематике Си/Си++.
4. О том, что умеет PVS-Studio. Ошибки, обнаруженные в Open Source проектах разработчиками PVS-Studio с помощью статического анализа.