Святослав Размыслов

22 Янв 2016

Теги:

#Cpp

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

22 Янв 2016

Автор: Святослав Размыслов

Введение
Разные ошибки
Опасные указатели
Остановись! Проверь Assert!
Заключение

В декабре 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()" с " 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.

#Cpp