Microsoft recently open-sourced ChakraCore, the key component of the high performance JavaScript engine that powers Microsoft Edge and Windows applications written in HTML/CSS/JS. This prompted PVS-Studio to apply its static analysis tool to the ChakraCore source code, revealing some interesting code fragments as a result.

The Chakra JavaScript engine was first introduced in 2010 to power IE 9 and is now a core component of Edge, Microsoft's replacement browser. It is also used in a range of Microsoft projects including Universal Windows 10 apps and it has even been used to implement Node.js without the customary V8 engine. The intention to open source it was announced at the JSConf US in December 2015 and a month later it was made available under an MIT License at the ChakraCore GitHub repository.

PVS-Studio is a static analyzer for bug detection in the source code of programs, written in C, C++ and C#. PVS-Studio tool is designed for developers of contemporary applications and integrates into the Visual Studio environments of 2010-2015.

Errors Revealed

As with other open source projects, this report only covers a limited number of all of the warnings issued by the analyzer, therefore PVS-Studio recommends the authors of the project to run the analyzer on their code themselves and study complete analysis results. Developers of open source projects can be provided with a temporary key for this purpose. 

The warnings in bold below are those provided by the analyzer. After the code snippet you'll find an explanation of how the bug could have arisen and in most cases how to eliminate it. To know more about the diagnostics with additional examples of bugs, click on the links to go to the PVS-Studio documentation.

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(....);
 }
 ....
}

There are two similar checks in the condition. Probably, while writing the code, a programmer accidentally chose the same constant in the IntelliSense menu, for instance, instead of "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())
 );
  ....
}

Two more identical comparisons. Most likely "srcIndexOpnd->GetStackSym()" was meant to be compared with " 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;
}

The analyzer detected two similar checks in the cascade of conditional operators; as a result a code block in the last condition never gets control. Full code of the given example is very long and it's hard to notice the mistake. It is a good example of the benefits of using a static code analyzer during the work with similar code, when the programmer can become tired and let their guard down.

Most likely, the last two conditions were meant to be like this:

 ....
 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(....);
 }
 ....
}

Dereference of "scriptContext" pointer is done before its validity is checked. It was luck that the bug didn't affect the program and therefore wasn't noticed. Such bugs can live in the code for a very long time and show up in rare atypical situations.

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; // <=
}

It's very suspicious that the same value is stored in the 'isInlined' boolean variable. Most likely the programmer intended to write something else.

Here is one more fragment, where the variable is assigned to itself: 

• 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;
   }
  }
 }
 ....
}

The analyzer detected, that a part of the (sub[i] != '-') conditional expression doesn't influence the check result. To make sure, let's have a look at the truth table. Most likely there is a misprint here; it's hard to say how to write this code correctly.