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

Введение

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

PVS-Studio ^[2] — это статический анализатор для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Инструмент PVS-Studio предназначен для разработчиков современных приложений и интегрируется в среды Visual Studio 2010-2015.

Подготавливая статью о проверке очередного открытого проекта ^[3], мы приводим в ней информацию далеко не о всех предупреждениях, которые выдаёт статический анализатор. Поэтому мы рекомендуем авторам проекта самостоятельно выполнить анализ и изучить все выдаваемые анализатором сообщения. Мы предоставляем на время ключ разработчикам открытых проектов.

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

V501 ^[4] 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 ^[4] 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 ^[4] 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 ^[5] 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 ^[6] 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 ^[7] 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 ^[8] 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();
}

Программисты часто ошибаются ^[9], пытаясь явно вызвать конструктор для инициализации объекта. В данном примере создаются новые неименованные объекты типа «StringCopyInfo» и тут же разрушаются. В результате поля класса остаются неинициализированными.

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

V610 ^[10] 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 ^[11] 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 находит участки кода, где выполняется разыменование указателя до его проверки на ноль. Обычно в проверяемых проектах всегда находится некоторое количество таких предупреждений. В нашей базе ошибок она занимает первое место по количеству найденных недочётов (см. примеры ^[12]). Но дело в том, что диагностики V595 несколько скучны, чтобы выписывать много примеров из какого-то проекта. Также проверка и разыменование указателя могут находится довольно далеко в коде функции: в десятках или даже сотнях строк друг от друга, что усложняет описание ошибки в статье.

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

V595 ^[13] 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 ^[13] 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 ^[13] 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 ^[14] 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 ^[15] 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 ^[15] 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 ^[15] 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 замечается позитивная тенденция открывать свои проекты под свободными лицензиями. Для нас это дополнительное тестирование анализатора на новых проектах, а также возможность продемонстрировать полезность и эффективность статического анализа на проектах такого крупного и известного производителя программного обеспечения.

Возможно, вам будет интересен список всех проверенных проектов ^[3], включающих и другие проекты от Microsoft, таких как .NET CoreCLR, .NET CoreFX и Microsoft Code Contracts.

Автор: PVS-Studio

Источник ^[16]