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

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

![0370_ChakraCore_ru/image1.png](https://import.viva64.com/docx/blog/0370_ChakraCore_ru/image1.png)

## Введение

[ChakraCore](https://github.com/Microsoft/ChakraCore) это базовая составляющая Chakra, высокопроизводительный движок JavaScript, который запускает приложения Microsoft Edge и Windows, написанные на HTML/CSS/JS\. ChakraCore поддерживает JIT\-компиляцию на JavaScript для x86/x64/ARM, сборку мусора и широкий спектр самых последних возможностей JavaScript\.

[PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) \- это статический анализатор для выявления ошибок в исходном коде программ, написанных на языках С, C\+\+ и C\#\. Инструмент PVS\-Studio предназначен для разработчиков современных приложений и интегрируется в среды Visual Studio 2010\-2015\.

Подготавливая статью о проверке очередного [открытого проекта](https://pvs-studio.ru/ru/blog/inspections/), мы приводим в ней информацию далеко не о всех предупреждениях, которые выдаёт статический анализатор\. Поэтому мы рекомендуем авторам проекта самостоятельно выполнить анализ и изучить все выдаваемые анализатором сообщения\. Мы предоставляем на время ключ разработчикам открытых проектов\.

## Разные ошибки

![0370_ChakraCore_ru/image2.png](https://import.viva64.com/docx/blog/0370_ChakraCore_ru/image2.png)

[V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions 'this\-\>propId \=\= Js::PropertyIds::\_superReferenceSymbol' to the left and to the right of the '\|\|' operator\. diagobjectmodel\.cpp 123

```cpp
IDiagObjectModelDisplay * ResolvedObject::CreateDisplay()
{
  ....
  if (this->isConst ||
    this->propId == Js::PropertyIds::_superReferenceSymbol ||
    this->propId == Js::PropertyIds::_superReferenceSymbol)
  {
      pOMDisplay->SetDefaultTypeAttribute(....);
  }
  ....
}
```

В условии присутствует две одинаковые проверки\. Возможно, при написании кода, в меню IntelliSense случайно была выбрана такая же константа, например, вместо "Js::PropertyIds:: \_superCtorReferenceSymbol"\.

[V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions 'GetVarSymID\(srcIndexOpnd\-\>GetStackSym\(\)\)' to the left and to the right of the '\=\=' operator\. globopt\.cpp 20795

```cpp
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\(\)" с " srcBaseOpnd \-\>GetStackSym\(\)"\.

[V517](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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;
}
```

В каскаде условных операторов найдены две одинаковые проверки, вследствие чего, блок кода в последнем условии никогда не получает управления\. Полный код представленного примера очень велик и в нём сложно заметить опечатку\. Это хороший пример, показывающий как бывает полезен статического анализатор при работе с однотипным кодом, от которого программист быстро устаёт и теряет внимательность\.

Скорее всего, последние два условия хотели написать так:

```cpp
....
else if (srcReg2 && (srcReg2->m_sym->m_isStrEmpty))
{
  ....
}
else if (srcReg1 && (srcReg1->m_sym-> m_isStrEmpty))       // <=
{
  ....
}
```

[V713](https://pvs-studio.ru/ru/docs/warnings/v713/) The pointer scriptContext was utilized in the logical expression before it was verified against nullptr in the same logical expression\. diaghelpermethodwrapper\.cpp 214

```cpp
template <bool doCheckParentInterpreterFrame>
void HandleHelperOrLibraryMethodWrapperException(....)
{
  ....
  if (!exceptionObject->IsDebuggerSkip() ||
    exceptionObject == scriptContext->GetThreadContext()->.... ||
    exceptionObject == scriptContext->GetThreadContext()->.... ||
    !scriptContext)    // <=
  {
    throw exceptionObject->CloneIfStaticExceptionObject(....);
  }
  ....
}
```

Разыменование указателя "scriptContext" выполняется раньше, чем проверяется его корректность\. Благодаря везению, такая ошибка ещё не проявила себя и не была замечена\. Подобные ошибки могут очень долго жить в коде, и проявляют себя в редких нетипичных ситуациях\.

[V570](https://pvs-studio.ru/ru/docs/warnings/v570/) The 'this\-\>isInlined' variable is assigned to itself\. functioncodegenjittimedata\.h 625

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting the 'sub\[i\] \!\= '\-' && sub\[i\] \=\= '/'' expression\. The expression is excessive or contains a misprint\. rl\.cpp 1388

```cpp
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\] \!\= '\-'\) не влияет на результат проверки\. Чтобы в этом убедиться, построим таблицу истинности\. Скорее всего здесь имеет место какая\-то опечатка, но каким должен быть правильный код, я затрудняюсь ответить\.

![0370_ChakraCore_ru/image3.png](https://import.viva64.com/docx/blog/0370_ChakraCore_ru/image3.png)

[V603](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
void StringCopyInfo::InstantiateForceInlinedMembers()
{
    AnalysisAssert(false);

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

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

Программисты часто [ошибаются](https://pvs-studio.ru/ru/blog/posts/cpp/0127/), пытаясь явно вызвать конструктор для инициализации объекта\. В данном примере создаются новые неименованные объекты типа "StringCopyInfo" и тут же разрушаются\. В результате поля класса остаются неинициализированными\.

Правильным вариантом было бы создать функцию инициализации и вызывать её из конструкторов и в этом месте\.

[V610](https://pvs-studio.ru/ru/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<'\. The left operand '\-1' is negative\. constants\.h 39

```cpp
class Constants
{
public:
  ....
  static const int Int31MinValue = -1 << 30;
  ....
};
```

Согласно современному стандарту языка C\+\+, сдвиг отрицательного числа приводит к неопределённому поведению\.

[V557](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The value of 'i' index could reach 8\. rl\.cpp 2375

```cpp
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 в конце:

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

Но может быть и пропустили какую\-нибудь строку в середине массива\!

## Опасные указатели

![0370_ChakraCore_ru/image4.png](https://import.viva64.com/docx/blog/0370_ChakraCore_ru/image4.png)

Диагностика V595 находит участки кода, где выполняется разыменование указателя до его проверки на ноль\. Обычно в проверяемых проектах всегда находится некоторое количество таких предупреждений\. В нашей базе ошибок она занимает первое место по количеству найденных недочётов \(см\. [примеры](https://pvs-studio.ru/ru/blog/examples/v595/)\)\. Но дело в том, что диагностики V595 несколько скучны, чтобы выписывать много примеров из какого\-то проекта\. Также проверка и разыменование указателя могут находится довольно далеко в коде функции: в десятках или даже сотнях строк друг от друга, что усложняет описание ошибки в статье\.

Далее я опишу всего несколько наиболее коротких и наглядных примеров кода, в которых скорее всего присутствует ошибка при работе с указателями\.

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'instrLd' pointer was utilized before it was verified against nullptr\. Check lines: 1823, 1831\. flowgraph\.cpp 1823

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'src2Val' pointer was utilized before it was verified against nullptr\. Check lines: 9717, 9725\. globopt\.cpp 9717

```cpp
bool GlobOpt::TypeSpecializeIntBinary(....)
{
  ....
  bool isIntConstMissingItem = src2Val->GetValueInfo()->....

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

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

Указатель "src2Val" используется в начале функции, но потом разработчики активно начали проверять, является ли указатель равным нулю\.

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'm\_lastInstr' pointer was utilized before it was verified against nullptr\. Check lines: 214, 228\. irbuilderasmjs\.cpp 214

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v522/) Dereferencing of the null pointer 'tempNumberTracker' might take place\. backwardpass\.cpp 578

```cpp
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\!

![0370_ChakraCore_ru/image5.png](https://import.viva64.com/docx/blog/0370_ChakraCore_ru/image5.png)

Assert, размещённый в программе, указывает на то, что разработчик предполагает, что некоторое выражение является истинным для корректно работающей программы\. Но можно ли доверять таким "успешным" проверкам?

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'srcIndex \- src\-\>left \>\= 0' is always true\. Unsigned type value is always \>\= 0\. sparsearraysegment\.inl 355

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression is always true\. Probably the '&&' operator should be used here\. bytecodegenerator\.cpp 805

```cpp
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" ложно, то следующие условия всегда истинны\. В этом можно убедиться, построив таблицу истинности:

![0370_ChakraCore_ru/image6.png](https://import.viva64.com/docx/blog/0370_ChakraCore_ru/image6.png)

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'callSiteId \>\= 0' is always true\. Unsigned type value is always \>\= 0\. inline\.cpp 1181

```cpp
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 замечается позитивная тенденция открывать свои проекты под свободными лицензиями\. Для нас это дополнительное тестирование анализатора на новых проектах, а также возможность продемонстрировать полезность и эффективность статического анализа на проектах такого крупного и известного производителя программного обеспечения\.

Возможно, вам будет интересен список всех [проверенных проектов](https://pvs-studio.ru/ru/blog/inspections/), включающих и другие проекты от Microsoft, таких как \.NET CoreCLR, \.NET CoreFX и Microsoft Code Contracts\.