ChakraCore: перевірка JavaScript-движка для Microsoft Edge

У грудні 2015 року на конференції JSConf US розробники оголосили, що планують відкрити вихідний код ключових компонентів JavaScript-движка Chakra, що працює в Microsoft Edge. Нещодавно вихідний код ChackraCore під ліцензією MIT опублікували у відповідному сховищі на GitHub. У статті я розповім, що вдалося знайти цікавого в проекті за допомогою статичного аналізатора PVS-Studio.

Введення
ChakraCore це базова складова Chakra, високопродуктивний движок JavaScript, який запускає програми Microsoft Edge і Windows, написані на HTML/CSS/JS. ChakraCore підтримує JIT-компіляції JavaScript для x86/x64/ARM, збирання сміття і широкий спектр самих останніх можливостей JavaScript.

PVS-Studio — це статичний аналізатор для виявлення помилок у вихідному коді програм, написаних на мовах С, C++ і C#. Інструмент PVS-Studio призначений для розробників сучасних програм і інтегрується в середовища Visual Studio 2010-2015.

Готуючи статтю про перевірку чергового відкритого проекту, ми наводимо інформацію далеко не про всі попередження, які видає статичний аналізатор. Тому ми рекомендуємо авторам проекту самостійно виконати аналіз та вивчити всі видані аналізатором повідомлення. Ми надаємо на час ключ розробникам відкритих проектів.

Різні помилки


V501 There are identical sub-expressions 'this->propId == Js::PropertyIds::_superReferenceSymbol' to the left and to the right of the '||' operator. diagobjectmodel.cpp 123
IDiagObjectModelDisplay * ResolvedObject::CreateDisplay()
{
....
if (this->isConst ||
this->propId == Js::PropertyIds::_superReferenceSymbol ||
this->propId == Js::PropertyIds::_superReferenceSymbol)
{
pOMDisplay->SetDefaultTypeAttribute (....);
}
....
}

В умові присутні дві однакові перевірки. Можливо, при написанні коду, в меню IntelliSense випадково була обрана така ж константа, наприклад, замість «Js::PropertyIds:: _superCtorReferenceSymbol».

V501 There are identical sub-expressions 'GetVarSymID(srcIndexOpnd->GetStackSym())' to the left and to the right of the '==' operator. globopt.cpp 20795
void GlobOpt::EmitMemop (....)
{
....
IR::RegOpnd *srcBaseOpnd = nullptr;
IR::RegOpnd *srcIndexOpnd = nullptr;
IRType srcType;
GetMemOpSrcInfo (...., srcBaseOpnd, srcIndexOpnd, srcType);
Assert(GetVarSymID(srcIndexOpnd->GetStackSym()) == // <=
GetVarSymID(srcIndexOpnd->GetStackSym())); // <=
....
}

Ще два однакових порівняння. Швидше за все, хотіли порівняти «srcIndexOpnd->GetStackSym()» c " srcBaseOpnd ->GetStackSym()".

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 3220, 3231. lower.cpp 3220
bool Lowerer::GenerateFastBrSrEq (....,
IR::RegOpnd * srcReg1,
IR::RegOpnd * srcReg2,
....)
{
if (srcReg2 && IsConstRegOpnd(srcReg2))
{
....
}
else if (srcReg1 && IsConstRegOpnd(srcReg1))
{
....
}
else if (srcReg2 && (srcReg2->m_sym->m_isStrConst))
{
....
}
else if (srcReg1 && (srcReg1->m_sym->m_isStrConst)) // <=
{
....
}
else if (srcReg2 && (srcReg2->m_sym->m_isStrEmpty))
{
....
}
else if (srcReg1 && (srcReg1->m_sym->m_isStrConst)) // <=
{
....
}

return false;
}

У каскаді умовних операторів знайдені дві однакові перевірки, внаслідок чого, блок коду в останньому умови ніколи не отримує управління. Повний код поданого прикладу дуже великий і в ньому важко помітити помилку. Це хороший приклад, який показує як буває корисний статичного аналізатор при роботі з однотипним кодом, від якого програміст швидко втомлюється і втрачає пильність.

Швидше за все, останні дві умови хотіли написати так:
....
else if (srcReg2 && (srcReg2->m_sym->m_isStrEmpty))
{
....
}
else if (srcReg1 && (srcReg1->m_sym-> m_isStrEmpty)) // <=
{
....
}

V713 The pointer scriptContext was utilized in the logical expression before it was verified against nullptr in the same logical expression. diaghelpermethodwrapper.cpp 214
template <bool doCheckParentInterpreterFrame>
void HandleHelperOrLibraryMethodWrapperexception (....)
{
....
if (!exceptionObject->IsDebuggerSkip() ||
exceptionObject == scriptContext->GetThreadContext()->.... ||
exceptionObject == scriptContext->GetThreadContext()->.... ||
!scriptContext) // <=
{
throw exceptionObject->CloneIfStaticExceptionObject (....);
}
....
}

Розіменування вказівника «scriptContext» виконується раніше, ніж перевіряється його коректність. Завдяки везінню, така помилка ще не проявила себе і не була помічена. Подібні помилки можуть дуже довго жити в коді, і виявляють себе в окремих нетипових ситуаціях.

V570 The 'this->isInlined' variable is assigned to itself. functioncodegenjittimedata.h 625
void SetupRecursiveInlineeChain(
Recycler *const recycler,
const ProfileId profiledCallSiteId)
{
if (!inlinees)
{
inlinees = RecyclerNewArrayZ (....);
}
inlinees[profiledCallSiteId] = this;
inlineeCount++;
this->isInlined = isInlined; // <=
}

Дуже підозріло, що в булевскую змінну 'isInlined' кладеться теж саме значення. Швидше за все хотіли написати щось інше.

Ще одне місце присвоювання змінної самій собі:
  • V570 The 'sym->m_isTaggableIntConst' variable is assigned to itself. linearscan.cpp 3170
V590 Consider inspecting the 'sub[i] != '-' && sub[i] == '/" expression. The expression is excessive or contains a misprint. rl.cpp 1388
const char *
stristr
(
const char * str,
const char * sub
)
{
....
for (i = 0; i < len; i++)
{
if (tolower(str[i]) != tolower(sub[i]))
{
if ((str[i] != '/' && str[i] != '-') ||
(sub[i] != '-' && sub[i] == '/')) { / <=
// if the mismatch is not between '/' and '-'
break;
}
}
}
....
}

Аналізатор виявив, що частина виразу (sub[i] != '-') не впливає на результат перевірки. Щоб переконатися в цьому, побудуємо таблицю істинності. Швидше за все тут має місце якась помилка, але яким повинен бути правильний код, мені важко відповісти.



V603 The object was created but it is not being used. If you wish to call constructor, 'this->StringCopyInfo::StringCopyInfo (....)' should be used. stringcopyinfo.cpp 64
void StringCopyInfo::InstantiateForceInlinedMembers()
{
AnalysisAssert(false);

StringCopyInfo copyInfo;
JavascriptString *const string = nullptr;
wchar_t *const buffer = nullptr;

(StringCopyInfo()); // <=
(StringCopyInfo(string, buffer)); // <=
copyInfo.SourceString();
copyInfo.DestinationBuffer();
}

Програмісти часто помиляються, явно намагаючись викликати конструктор для ініціалізації об'єкту. В даному прикладі створюються нові неіменовані об'єкти типу «StringCopyInfo» і тут же руйнуються. В результаті поля класу залишаються неинициализированными.

Правильним варіантом було б створити функцію ініціалізації і викликати її з конструкторів і в цьому місці.

V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. constants.h 39
class Constants
{
public:
....
static const int Int31MinValue = -1 << 30;
....
};

Згідно з сучасним стандартом мови C++, зсув від'ємного числа призводить до невизначеної поведінки.

V557 Array overrun is possible. The value of 'i' index could reach 8. rl.cpp 2375
enum TestInfoKind::_TIK_COUNT = 9

const char * const TestInfoEnvLstFmt[] =
{
" TESTFILE=\"%s"",
" BASELINE=\"%s"",
" CFLAGS=\"%s"",
" LFLAGS=\"%s"",
NULL,
NULL,
NULL,
NULL // <= TestInfoEnvLstFmt[7]
};

void
WriteEnvLst
(
Test * pDir, TestList * pTestList
)
{
....
// print the other TIK_*
for(int i=0;i < _TIK_COUNT; i++) {
if (variants->testInfo.data[i] && TestInfoEnvLstFmt[i]){// <=
LstFilesOut->Add(TestInfoEnvLstFmt[i], // <=
variants->testInfo.data[i]);
}
....
}
....
}

Аналізатор виявив вихід індексу за межі масиву. Справа в тому, що цикл for() виконує 9 ітерацій, а в масиві «TestInfoEnvLstFmt[]» всього 8 елементів.

Швидше за все забули додати ще один NULL в кінці:
const char * const TestInfoEnvLstFmt[] =
{
" TESTFILE=\"%s"",
" BASELINE=\"%s"",
" CFLAGS=\"%s"",
" LFLAGS=\"%s"",
NULL,
NULL,
NULL,
NULL // <= TestInfoEnvLstFmt[7]
NULL // <= TestInfoEnvLstFmt[8]
};

Але може бути і пропустили якусь рядок в середині масиву!

Небезпечні покажчики


Діагностика V595 знаходить ділянки коду, де виконується розіменування вказівника до його перевірки на нуль. Зазвичай перевіряються проектах завжди знаходиться деяка кількість таких попереджень. В нашій базі помилок вона займає перше місце за кількістю знайдених недоліків (див. приклади). Але справа в тому, що діагностики V595 кілька нудні, щоб виписувати багато прикладів з якогось проекту. Також перевірка та розіменування вказівника можуть знаходиться досить далеко в коді функції: у десятках чи навіть сотнях рядків один від одного, що ускладнює опис помилки в статті.

Далі я опишу лише кілька найбільш коротких і наочних прикладів коду, в яких швидше за все присутня помилка при роботі з покажчиками.

V595 The 'instrLd' pointer was utilized before it was verified against nullptr. Check lines: 1823, 1831. flowgraph.cpp 1823
IR::Instr *
FlowGraph::PeepTypedCm(IR::Instr *instr)
{
....
if (instrLd && !instrLd->GetSrc1()->IsEqual(instr->GetDst()))
{
return nullptr;
}

if(instrLd2 && !instrLd2->GetSrc1()->IsEqual(instrLd->GetDst()))
{
return nullptr;
}
....
}

Зверніть увагу на покажчик з назвою «instrLd». У першому умови розіменування вказівника виконується в парі з перевіркою на нуль, але в другому умови це зробити забули, тому може виникнути ситуація, коли відбудеться розіменування нульового покажчика.

V595 The 'src2Val' pointer was utilized before it was verified against nullptr. Check lines: 9717, 9725. globopt.cpp 9717
bool GlobOpt::TypeSpecializeIntBinary (....)
{
....
bool isIntConstMissingItem = src2Val->GetValueInfo()->....

if(isIntConstMissingItem)
{
isIntConstMissingItem = Js::SparseArraySegment<int>::....
}

if (!src2Val || !(src2Val->GetValueInfo()->IsLikelyInt()) ||
isIntConstMissingItem)
{
return false;
}
....
}

Покажчик «src2Val» використовується на початку функції, але потім розробники активно почали перевіряти, чи є вказівник рівним нулю.

V595 The 'm_lastInstr' pointer was utilized before it was verified against nullptr. Check lines: 214, 228. irbuilderasmjs.cpp 214
void
IRBuilderAsmJs::AddInstr(IR::Instr * instr, uint32 offset)
{
m_lastInstr->InsertAfter(instr); // <=
if (offset != Js::Constants::NoByteCodeOffset)
{
....
}
else if (m_lastInstr) // <=
{
instr->SetByteCodeOffset(m_lastInstr->GetByteCodeOffset());
}
m_lastInstr = instr;
....
}

Ще один приклад недбалого використання покажчика, який потенційно може бути нульовим.

Список схожих місць:
  • V595 The 'arrayData' pointer was utilized before it was verified against nullptr. Check lines: 868, 870. immutablelist.h 868
  • V595 The 'pMembersList' pointer was utilized before it was verified against nullptr. Check lines: 2012, 2015. diagobjectmodel.cpp 2012
  • V595 The 'walkerRef' pointer was utilized before it was verified against nullptr. Check lines: 3191, 3193. diagobjectmodel.cpp 3191
  • V595 The 'block->loop' pointer was utilized before it was verified against nullptr. Check lines: 981, 1002. globopt.cpp 981
  • V595 The 'src2Val' pointer was utilized before it was verified against nullptr. Check lines: 12528, 12536. globopt.cpp 12528
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 1966, 1967. irbuilderasmjs.cpp 1966
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 2010, 2011. irbuilderasmjs.cpp 2010
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 2076, 2077. irbuilderasmjs.cpp 2076
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 3591, 3592. irbuilderasmjs.cpp 3591
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 4113, 4114. irbuilderasmjs.cpp 4113
  • V595 The 'symDst' pointer was utilized before it was verified against nullptr. Check lines: 4510, 4511. irbuilderasmjs.cpp 4510
  • V595 The 'm_lastInstr' pointer was utilized before it was verified against nullptr. Check lines: 1102, 1116. irbuilder.cpp 1102
У списку представлені найбільш прості і зрозумілі приклади. Для вивчення всіх подібних місць розробникам слід самим вивчити звіт аналізатора.

V522 Dereferencing of the null pointer 'tempNumberTracker' might take place. backwardpass.cpp 578
void
BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
{
TempNumberTracker * tempNumberTracker = nullptr; // <= line 346
....
if (!block->isDead)
{
....
if(!IsCollectionPass())
{
....
if (this->DoMarkTempNumbers())
{
tempNumberTracker = JitAnew(....); // <= line 413
}
....
....
if (blockSucc->tempNumberTracker != nullptr)
{
....
tempNumberTracker->MergeData(....); // <= line 578
if (deleteData)
{
blockSucc->tempNumberTracker = nullptr;
}
}
....
}

Інший приклад діагностики, але теж про покажчики. Тут представлений фрагмент функції MergeSuccBlocksInfo(), вона досить довга — 707 рядків. Але з допомогою статичного аналізу вдалося знайти покажчик «tempNumberTracker», ініціалізація якого потенційно може не виконається з-за ряду умов. В результаті при невдалому збігу обставин відбудеться розіменування нульового покажчика.

Зупинись! Перевір Assert!


Assert, розміщений у програмі, вказує на те, що розробник припускає, що деякий вираз є істинним для коректно працюючої програми. Але чи можна довіряти таким «успішним» перевіркам?

V547 Expression 'srcIndex — src->left >= 0' is always true. Unsigned type value is always >= 0. sparsearraysegment.inl 355
class SparseArraySegmentBase
{
public:
static const uint32 MaxLength;
....
uint32 size;
....
}

template < typename T>
SparseArraySegment<T>* SparseArraySegment<T>::CopySegment (....,
uint32 srcIndex, ....)
{
....
AssertMsg(srcIndex - src->left >= 0, // <=
"src->left > srcIndex resulting in \
negative indexing of src->elements");
js_memcpy_s(dst->elements + dstIndex - dst->left,
sizeof(T) * inputLen,
src->elements + srcIndex - src->left,
sizeof(T) * inputLen);
return dst;
}

Зверніть увагу на порівняння «srcIndex — src->left >= 0». Різниця двох беззнаковим чисел завжди буде більше або дорівнює нулю. Далі ця формула використовується в функції для роботи з пам'яттю. Результат може бути не таким, як очікував програміст.

V547 Expression is always true. Probably the '&&' operator should be used here. bytecodegenerator.cpp 805
void ByteCodeGenerator::AssignRegister(Symbol *sym)
{
AssertMsg(sym->GetDecl() == nullptr ||
sym->GetDecl()->nop != knopConstDecl|| // <=
sym->GetDecl()->nop != knopLetDecl, "...."); // <=

if (sym->GetLocation() == Js::Constants::NoRegister)
{
sym->SetLocation(NextVarRegister());
}
}

У цьому Assert'е тестування деяких значень виконується частково. Якщо припущення «sym->GetDecl() == nullptr» хибне, то наступні умови завжди істинні. У цьому можна переконатися, побудувавши таблицю істинності:



V547 Expression 'callSiteId >= 0' is always true. Unsigned type value is always >= 0. inline.cpp 1181
typedef uint16 ProfileId;

Func * Inline::BuildInlinee(Js::FunctionBody* funcBody, ....)
{
....
Js::ProfileId callSiteId = static_cast<Js::ProfileId> (....);
Assert(callSiteId >= 0);
....
}

В цьому і ще двох інших місцях аналізатор виявив некоректне порівняння беззнакового числа з нулем:
  • V547 Expression 'callSiteId >= 0' is always true. Unsigned type value is always >= 0. inline.cpp 2627
  • V547 Expression 'callSiteId >= 0' is always true. Unsigned type value is always >= 0. inline.cpp 3657
Висновок
У Microsoft помічається позитивна тенденція відкривати свої проекти під вільними ліцензіями. Для нас це додаткове тестування аналізатора на нових проектах, а також можливість продемонструвати корисність і ефективність статичного аналізу на проектах такого великого та відомого виробника програмного забезпечення.

Можливо, вам буде цікавий список всіх перевірених проектів, що включають і інші проекти від Microsoft, таких як .NET CoreCLR, .NET CoreFX і Microsoft Code Contracts.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Svyatoslav Razmyslov. ChakraCore: analysis of JavaScript-engine for Microsoft Edge.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.