PVS-Studio: 25 подозрительных фрагментов кода из CoreCLR
Корпорация Microsoft выложила в открытый доступ исходный код движка CoreCLR, который является ключевым элементом .NET Core. Эта новость, конечно же, не могла не привлечь наше внимание. Ведь чем больше аудитория у проекта, тем тревожнее будут выглядеть найденные подозрительные места. Несмотря на авторство Microsoft, как в любом крупном проекте, тут есть на что посмотреть и над чем задуматься.Введение
CoreCLR является средой исполнения .NET Core, выполняя такие функции как сборку мусора или компиляции в конечный машинный код. .Net Core — это модульная реализация .Net, которая может быть использована как база для огромного количества сценариев.Исходный код с недавнего времени доступен на GitHub и проверялся с помощью PVS-Studio 5.23. Как и я, желающие могут получить полный лог проверки с помощью Microsoft Visual Studio Community Edition, выход которой тоже был недавней новостью от Microsoft.
Опечатки Традиционно я начинаю с мест, похожих на опечатки. В условных выражениях повторяются переменные, константы, макросы или поля структур/классов. Наличие ошибки — предмет дискуссий, тем не менее такие места были найдены и выглядят подозрительно.V501 There are identical sub-expressions 'tree→gtOper == GT_CLS_VAR' to the left and to the right of the '||' operator. ClrJit lsra.cpp 3140
// register variable GTNODE (GT_REG_VAR, «regVar» ,0, GTK_LEAF|GTK_LOCAL) // static data member GTNODE (GT_CLS_VAR, «clsVar» ,0, GTK_LEAF) // static data member address GTNODE (GT_CLS_VAR_ADDR,»&clsVar» ,0, GTK_LEAF) …
void LinearScan: buildRefPositionsForNode (GenTree *tree, …) { … if ((tree→gtOper == GT_CLS_VAR || tree→gtOper == GT_CLS_VAR) && i == 1) { registerType = TYP_PTR; currCandidates = allRegs (TYP_PTR); } … } Хотя структура 'GenTree' имеет похожее по имени поле «tree→gtType», но оно имеет другой тип с «tree→gtOper». Думаю, здесь дело в скопированной константе. То-есть в выражении должна использоваться ещё одна константа, помимо GT_CLS_VAR.V501 There are identical sub-expressions 'DECODE_PSP_SYM' to the left and to the right of the '|' operator. daccess 264
enum GcInfoDecoderFlags { DECODE_SECURITY_OBJECT = 0×01, DECODE_CODE_LENGTH = 0×02, DECODE_VARARG = 0×04, DECODE_INTERRUPTIBILITY = 0×08, DECODE_GC_LIFETIMES = 0×10, DECODE_NO_VALIDATION = 0×20, DECODE_PSP_SYM = 0×40, DECODE_GENERICS_INST_CONTEXT = 0×80, DECODE_GS_COOKIE = 0×100, DECODE_FOR_RANGES_CALLBACK = 0×200, DECODE_PROLOG_LENGTH = 0×400, DECODE_EDIT_AND_CONTINUE = 0×800, };
size_t GCDump: DumpGCTable (PTR_CBYTE table, …) { GcInfoDecoder hdrdecoder (table, (GcInfoDecoderFlags)(DECODE_SECURITY_OBJECT | DECODE_GS_COOKIE | DECODE_CODE_LENGTH | DECODE_PSP_SYM //<==1 | DECODE_VARARG | DECODE_PSP_SYM //<==1 | DECODE_GENERICS_INST_CONTEXT //<==2 | DECODE_GC_LIFETIMES | DECODE_GENERICS_INST_CONTEXT //<==2 | DECODE_PROLOG_LENGTH), 0); .... } Тут даже две повторяющиеся константы, хотя в перечислении " GcInfoDecoderFlags" есть и другие константы, которые в условии не используются.Ещё похожие места:
V501 There are identical sub-expressions 'varLoc1.vlStk2.vls2BaseReg' to the left and to the right of the '==' operator. cee_wks util.cpp 657 V501 There are identical sub-expressions 'varLoc1.vlStk2.vls2Offset' to the left and to the right of the '==' operator. cee_wks util.cpp 658 V501 There are identical sub-expressions 'varLoc1.vlFPstk.vlfReg' to the left and to the right of the '==' operator. cee_wks util.cpp 661 V700 Consider inspecting the 'T foo = foo = …' expression. It is odd that variable is initialized through itself. cee_wks zapsig.cpp 172 BOOL ZapSig: GetSignatureForTypeHandle (…) { … CorElementType elemType = elemType = TryEncodeUsingShortcut (pMT); … } Вроде бы просто лишнее присваивание, но такие ошибки часто делают при копировании кода и забывают что-нибудь переименовать. В любом случае, в таком виде код бессмысленный.V523 The 'then' statement is equivalent to the 'else' statement. cee_wks threadsuspend.cpp 2468
enum __MIDL___MIDL_itf_mscoree_0000_0004_0001 { OPR_ThreadAbort = 0, OPR_ThreadRudeAbortInNonCriticalRegion = … , OPR_ThreadRudeAbortInCriticalRegion = …) , OPR_AppDomainUnload = … , OPR_AppDomainRudeUnload = (OPR_AppDomainUnload + 1) , OPR_ProcessExit = (OPR_AppDomainRudeUnload + 1) , OPR_FinalizerRun = (OPR_ProcessExit + 1) , MaxClrOperation = (OPR_FinalizerRun + 1) } EClrOperation;
void Thread: SetRudeAbortEndTimeFromEEPolicy () { LIMITED_METHOD_CONTRACT; DWORD timeout; if (HasLockInCurrentDomain ()) { timeout = GetEEPolicy ()→ GetTimeout (OPR_ThreadRudeAbortInCriticalRegion); //<== } else { timeout = GetEEPolicy()-> GetTimeout (OPR_ThreadRudeAbortInCriticalRegion); //<== } .... } Данная диагностика находит одинаковые блоки в конструкциях if/else. И здесь тоже есть подозрение на опечатку в константе. В первом случае, по смыслу как раз подходит «OPR_ThreadRudeAbortInNonCriticalRegion».Аналогичные места:
V523 The 'then' statement is equivalent to the 'else' statement. ClrJit instr.cpp 3427 V523 The 'then' statement is equivalent to the 'else' statement. ClrJit flowgraph.cpp 18815 V523 The 'then' statement is equivalent to the 'else' statement. daccess dacdbiimpl.cpp 6374 Список инициализации конструктора V670 The uninitialized class member 'gcInfo' is used to initialize the 'regSet' member. Remember that members are initialized in the order of their declarations inside a class. ClrJit codegencommon.cpp 92 CodeGenInterface *getCodeGenerator (Compiler *comp);
class CodeGenInterface { friend class emitter;
public: … RegSet regSet; //<=== line 91 .... public: GCInfo gcInfo; //<=== line 322 .... };
// CodeGen constructor CodeGenInterface: CodeGenInterface (Compiler* theCompiler) : compiler (theCompiler), gcInfo (theCompiler), regSet (theCompiler, gcInfo) { } Согласно стандарту, порядок инициализация членов класса в конструкторе происходит в порядке их объявления в классе. Для исправления ошибки следует перенести объявление члена класса 'gcInfo' выше объявления 'regSet'.Ложное, но полезное предупреждение V705 It is possible that 'else' block was forgotten or commented out, thus altering the program’s operation logics. daccess daccess.cpp 2979 HRESULT Initialize () { if (hdr.dwSig == sig) { m_rw = eRO; m_MiniMetaDataBuffSizeMax = hdr.dwTotalSize; hr = S_OK; } else // when the DAC initializes this for the case where the target is // (a) a live process, or (b) a full dump, buff will point to a // zero initialized memory region (allocated w/ VirtualAlloc) if (hdr.dwSig == 0 && hdr.dwTotalSize == 0 && hdr.dwCntStreams == 0) { hr = S_OK; } // otherwise we may have some memory corruption. treat this as // a liveprocess/full dump else { hr = S_FALSE; } … } Анализатор обнаружил подозрительное место в коде. Здесь видно, что код ПРОкомментирован и всё нормально. Но такого типа ошибки очень распространены, когда код после 'else' ЗАкомментирован и следующий за ним оператор становится частью условия. В данном примере нет ошибки, но её вполне можно допустить при редактировании этого места в будущем.64-битная ошибка V673 The '0xefefefef inline void Object: EnumMemoryRegions (void) { … SIZE_T size = sizeof (ObjHeader) + sizeof (Object); … size |= 0xefefefef << 28; .... } Про термин «64-битная ошибка» можно прочесть здесь. В данном примере, после сдвига будет выполняться операция «size |= 0xf0000000» в 32-х битной программе и «size |= 0x00000000f0000000» в 64-х битной. Скорее всего, в 64-х битной программе планировали вычислять: «size |= 0x0efefefef0000000». Но где же теряется старшая часть числа?Число «0xefefefef» имеет тип 'unsigned', так как не помещается в тип 'int'. Выполняется сдвиг 32-х битного числа и в результате мы получим 0xf0000000 беззнакового типа. Далее это беззнаковое число расширится до SIZE_T и мы получим 0x00000000f0000000.
Для корректной работы необходимо сначала выполнить явное приведение типа. Пример корректного кода:
inline void Object: EnumMemoryRegions (void) { … SIZE_T size = sizeof (ObjHeader) + sizeof (Object); … size |= SIZE_T (0xefefefef) << 28; .... } Ещё такое место: V673 The '0xefefefef << 28' expression evaluates to 1080581331517177856. 60 bits are required to store the value, but the expression evaluates to the 'unsigned' type which can only hold '32' bits. cee_dac dynamicmethod.cpp 807 Код «в отставке» Порой условия пишутся так, что буквально противоречат друг другу.V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 31825, 31827. cee_wks gc.cpp 31825
void gc_heap: verify_heap (BOOL begin_gc_p) { … if (brick_table [curr_brick] < 0) { if (brick_table [curr_brick] == 0) { dprintf(3, ("curr_brick %Ix for object %Ix set to 0", curr_brick, (size_t)curr_object)); FATAL_GC_ERROR(); } .... } .... } Код, который никогда не получает управление, но он выглядит не таким значимым, как в следующем примере:V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2353, 2391. utilcode util.cpp 2353
void PutIA64Imm22(UINT64 * pBundle, UINT32 slot, INT32 imm22) { if (slot == 0) { const UINT64 mask0 = UI64(0xFFFFFC000603FFFF); /* Clear all bits used as part of the imm22 */ pBundle[0] &= mask0;
UINT64 temp0; temp0 = (UINT64) (imm22 & 0×200000) << 20; // 1 s temp0 |= (UINT64) (imm22 & 0x1F0000) << 11; // 5 imm5c temp0 |= (UINT64) (imm22 & 0x00FF80) << 25; // 9 imm9d temp0 |= (UINT64) (imm22 & 0x00007F) << 18; // 7 imm7b /* Or in the new bits used in the imm22 */ pBundle[0] |= temp0; } else if (slot == 1) { .... } else if (slot == 0) //<== { const UINT64 mask1 = UI64(0xF000180FFFFFFFFF); /* Clear all bits used as part of the imm22 */ pBundle[1] &= mask1;
UINT64 temp1; temp1 = (UINT64) (imm22 & 0×200000) << 37; // 1 s temp1 |= (UINT64) (imm22 & 0x1F0000) << 32; // 5 imm5c temp1 |= (UINT64) (imm22 & 0x00FF80) << 43; // 9 imm9d temp1 |= (UINT64) (imm22 & 0x00007F) << 36; // 7 imm7b /* Or in the new bits used in the imm22 */ pBundle[1] |= temp1; } FlushInstructionCache(GetCurrentProcess(),pBundle,16); } Возможно, очень важный код никогда не получает управление из-за ошибки в каскаде условных операторов.Ещё подозрительные места:
V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 2898, 2900. daccess nidump.cpp 2898
V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 337, 339. utilcode prettyprintsig.cpp 337
V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 774, 776. utilcode prettyprintsig.cpp 774
Неопределённое поведение
V610 Undefined behavior. Check the shift operator '
inline static mdToken decodeToken (…)
{
//
if (ix >= cTokens) return rTokens[0]; return TokenFromRid (val >> m_cb[cTokens], rTokens[ix]); } Анализатор обнаружил операцию сдвига отрицательного числа, которая приводит к неопределенному поведению.V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0)' is negative. cee_dac decodemd.cpp 456
#define bits_generation 2 #define generation_mask (~(~0 << bits_generation))
#define MASK (len) (~((~0)< void Encoder: Add (unsigned value, unsigned length)
{
…
value = (value & MASK (length));
…
}
Благодаря сообщению анализатора V610 я нашёл несколько некорректных макросов. '~0' приводится к знаковому отрицательному числу типа int, после чего выполняется сдвиг. Как в одном из макросов, необходимо выполнить явное преобразование к unsigned:
#define bits_generation 2
#define generation_mask (~(~((unsigned int)0) << bits_generation)) #define MASK (len) (~((~((unsigned int)0))< V579 The Read function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. util.cpp 4943
HRESULT GetMTOfObject (TADDR obj, TADDR *mt)
{
if (! mt)
return E_POINTER; HRESULT hr = rvCache→Read (obj, mt, sizeof (mt), NULL);
if (SUCCEEDED (hr))
*mt &= ~3; return hr;
}
Семейство функций «memFAIL»
С использованием memXXX-функций можно допустить самые разные ошибки. Для поиска таких мест в анализаторе есть несколько диагностических правил.V512 A call of the 'memset' function will lead to underflow of the buffer 'pAddExpression'. sos strike.cpp 11973
DECLARE_API (Watch)
{
…
if (addExpression.data!= NULL || aExpression.data!= NULL)
{
WCHAR pAddExpression[MAX_EXPRESSION];
memset (pAddExpression, 0, MAX_EXPRESSION);
swprintf_s (pAddExpression, MAX_EXPRESSION, L»%S», …);
Status = g_watchCmd.Add (pAddExpression);
}
…
}
Распространённая ошибка, когда забывают делать поправку на размер типа:
WCHAR pAddExpression[MAX_EXPRESSION];
memset (pAddExpression, 0, sizeof (WCHAR)*MAX_EXPRESSION);
Еще несколько таких мест:
V512 A call of the 'memset' function will lead to underflow of the buffer 'pSaveName'. sos strike.cpp 11997
V512 A call of the 'memset' function will lead to underflow of the buffer 'pOldName'. sos strike.cpp 12013
V512 A call of the 'memset' function will lead to underflow of the buffer 'pNewName'. sos strike.cpp 12016
V512 A call of the 'memset' function will lead to underflow of the buffer 'pExpression'. sos strike.cpp 12024
V512 A call of the 'memset' function will lead to underflow of the buffer 'pFilterName'. sos strike.cpp 12039
V598 The 'memcpy' function is used to copy the fields of 'GenTree' class. Virtual table pointer will be damaged by this. ClrJit compiler.hpp 1344
struct GenTree
{
…
#if DEBUGGABLE_GENTREE
virtual void DummyVirt () {}
#endif // DEBUGGABLE_GENTREE
…
}; void GenTree: CopyFrom (const GenTree* src, Compiler* comp)
{
…
memcpy (this, src, src→GetNodeSize ());
…
}
Если объявлена переменная препроцессора 'DEBUGGABLE_GENTREE', то определяется виртуальная функция. Тогда класс содержит указатель на таблицу виртуальных методов и его уже нельзя вот так просто копировать.V598 The 'memcpy' function is used to copy the fields of 'GCStatistics' class. Virtual table pointer will be damaged by this. cee_wks gc.cpp 287
struct GCStatistics
: public StatisticsBase
{
…
virtual void Initialize ();
virtual void DisplayAndUpdate ();
…
}; GCStatistics g_LastGCStatistics; void GCStatistics: DisplayAndUpdate ()
{
…
memcpy (&g_LastGCStatistics, this, sizeof (g_LastGCStatistics));
…
}
В этом месте некорректное копирование выполняется не только в режиме отладки.V698 Expression 'memcmp (…) == -1' is incorrect. This function can return not only the value '-1', but any negative value. Consider using 'memcmp (…) < 0' instead. sos util.cpp 142
bool operator ()(const GUID& _Key1, const GUID& _Key2) const
{ return memcmp (&_Key1, &_Key2, sizeof (GUID)) == -1; }
Сравнивать результат функции 'memcmp' со значением 1 или -1 не корректно. Работоспособность таких конструкций зависит от библиотек, компилятора, его настроек, операционной системы, её разрядности и так далее; в таком случае необходимо проверять одно из трёх состояний: ' 0'.Аналогичное место:
V698 Expression 'wcscmp (…) == -1' is incorrect. This function can return not only the value '-1', but any negative value. Consider using 'wcscmp (…) < 0' instead. sos strike.cpp 3855
Про указатели
V522 Dereferencing of the null pointer 'hp' might take place. cee_wks gc.cpp 4488
heap_segment* gc_heap::get_segment_for_loh (size_t size
#ifdef MULTIPLE_HEAPS
, gc_heap* hp
#endif //MULTIPLE_HEAPS
)
{
#ifndef MULTIPLE_HEAPS
gc_heap* hp = 0;
#endif //MULTIPLE_HEAPS
heap_segment* res = hp->get_segment (size, TRUE);
…
}
Если 'MULTIPLE_HEAPS' не определён, то беда. Указатель окажется равен нулю.V595 The 'tree' pointer was utilized before it was verified against nullptr. Check lines: 6970, 6976. ClrJit gentree.cpp 6970
void Compiler: gtDispNode (GenTreePtr tree, …)
{
…
if (tree→gtOper >= GT_COUNT)
{
printf (» **** ILLEGAL NODE ****»);
return;
} if (tree && printFlags)
{
/* First print the flags associated with the node */
switch (tree→gtOper)
{
…
}
…
}
…
}
В исходном коде распространены места, когда валидность указателя таки проверяется, но уже после разыменования.Весь список: CoreCLR_V595.txt. Лишние проверки
Даже если лишний код не наносит вреда, то его наличие может просто отвлекать внимание разработки от более важных мест.V503 This is a nonsensical comparison: pointer >= 0. cee_wks gc.cpp 21707
void gc_heap: make_free_list_in_brick (BYTE* tree,
make_free_args* args)
{
assert ((tree >= 0));
…
}
Вот такая проверка указателя. Ещё примеры:
V503 This is a nonsensical comparison: pointer >= 0. cee_wks gc.cpp 23204
V503 This is a nonsensical comparison: pointer >= 0. cee_wks gc.cpp 27683
V547 Expression 'maxCpuId >= 0' is always true. Unsigned type value is always >= 0. cee_wks codeman.cpp 1219
void EEJitManager: SetCpuInfo ()
{
…
unsigned char buffer[16];
DWORD maxCpuId = getcpuid (0, buffer);
if (maxCpuId >= 0)
{
…
}
Похожий пример, только с типом DWORD.V590 Consider inspecting the 'wzPath[0] != L'\0' && wzPath[0] == L'\\'' expression. The expression is excessive or contains a misprint. cee_wks path.h 62
static inline bool
HasUncPrefix (LPCWSTR wzPath)
{
_ASSERTE (! clr: str: IsNullOrEmpty (wzPath));
return wzPath[0] != W ('\0') && wzPath[0] == W ('\\')
&& wzPath[1] != W ('\0') && wzPath[1] == W ('\\')
&& wzPath[2] != W ('\0') && wzPath[2] != W ('?');
}
Эту функцию можно упросить до такого варианта:
static inline bool
HasUncPrefix (LPCWSTR wzPath)
{
_ASSERTE (! clr: str: IsNullOrEmpty (wzPath));
return wzPath[0] == W ('\\')
&& wzPath[1] == W ('\\')
&& wzPath[2] != W ('\0')
&& wzPath[2] != W ('?');
}
Ещё такое место:
V590 Consider inspecting this expression. The expression is excessive or contains a misprint. cee_wks path.h 72
V571 Recurring check. The 'if (moduleInfo[MSCORWKS].baseAddr == 0)' condition was already verified in line 749. sos util.cpp 751
struct ModuleInfo
{
ULONG64 baseAddr;
ULONG64 size;
BOOL hasPdb;
}; HRESULT CheckEEDll ()
{
…
// Do we have clr.dll
if (moduleInfo[MSCORWKS].baseAddr == 0) //<==
{
if (moduleInfo[MSCORWKS].baseAddr == 0) //<==
g_ExtSymbols->GetModuleByModuleName (
MAIN_CLR_MODULE_NAME_A,0, NULL,
&moduleInfo[MSCORWKS].baseAddr);
if (moduleInfo[MSCORWKS].baseAddr!= 0 && //<==
moduleInfo[MSCORWKS].hasPdb == FALSE)
{
....
}
....
}
....
}
Во втором случае 'baseAddr' уже можно не проверять.V704 'this == nullptr' expression should be avoided — this expression is always false on newer compilers, because 'this' pointer can never be NULL. ClrJit gentree.cpp 12731
bool FieldSeqNode: IsFirstElemFieldSeq ()
{
if (this == nullptr)
return false;
return m_fieldHnd == FieldSeqStore: FirstElemPseudoField;
}
Согласно стандарту С++, указатель this никогда не может быть нулевым. О возможных последствиях такого кода можно подробно прочитать в описании диагностики V704. То, что такой код может работать корректно после компиляции компилятором Visual C++, это просто везение и полагаться на него по-честному нельзя.Весь список: CoreCLR_V704.txt. V668 There is no sense in testing the 'newChunk' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ClrJit stresslog.h 552
FORCEINLINE BOOL GrowChunkList ()
{
…
StressLogChunk * newChunk = new StressLogChunk (…);
if (newChunk == NULL)
{
return FALSE;
}
…
}
Если оператор 'new' не смог выделить память, то согласно стандарту языка Си++, генерируется исключение std: bad_alloc (). Таким образом проверять указатель на равенство нулю не имеет смысла.Лучше проверить такие места, вот полный список: CoreCLR_V668.txt. Заключение
Недавно открытый проект CoreCLR хороший пример того, как может выглядеть софт с закрытым исходным кодом. На эту тему постоянно ведутся дискуссии и вот вам ещё один повод для размышлений и обсуждений.Для нас же важно, что в любом большом проекте можно найти какие-то ошибки и самое лучшее применение статическому анализатору — это регулярные проверки. Не ленитесь, скачайте PVS-Studio и проверьте свой проект. Эта статья на английском
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. PVS-Studio: 25 Suspicious Code Fragments in CoreCLR.
