As an example of another diagnostic which is also connected with pointers, this is a code fragment of the MergeSuccBlocksInfo() function, which is quite long - 707 strings. But with the help of static analysis we managed to find "tempNumberTracker" pointer, whose initialization can potentially fail due to several conditions. As a result, if the odds are against the programmer, there will be a dereference of a null pointer. 

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

Stop! Check Assert!

Assert, located in the program indicates that a programmer assumes that some expression is true for a correctly running program. But can these "successful checks be trusted"?

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

Note the comparison "srcIndex-src-> left > = 0". The difference of two unsigned numbers will always be greater than or equal to zero. Further on this function is used for working with the memory. The result can be different from the one that the programmer expected.

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

In this Assert testing of some values is done only partially. If the expression "sym->GetDecl() == nullptr" is false, then the following conditions are always true. You can check it by building a truth table:

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

In this and couple of other fragments the analyzer detected incorrect comparison of an unsigned number with null: 

• 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

Conclusion

Increasingly Microsoft has adopted the policy of releasing its projects under free licenses. This gives PVS-Studio an additional opportunity to test its analyzer on new projects and demonstrate the usefulness and effectiveness of static code analysis on the projects of this large and well known software vendor.

Also from PVS-Studio on IProgrammer:

Test Your C++ Static Analysis Skills

Finding Bugs In The First C++ Compiler - What does Bjarne Think!

Related Articles

Microsoft Open Sources Chakra JavaScript Engine 

Comments

Make a Comment or View Existing Comments Using Disqus

or email your comment to: comments@i-programmer.info