Page MenuHomePhabricator

[llvm][clang] Add checks for the smart pointers with the possibility to be null
AbandonedPublic

Authored by OikawaKirie on Nov 19 2020, 11:35 PM.

Details

Summary

All these potential null pointer dereferences are reported by my static analyzer for null smart pointer dereferences, which has a different implementation from alpha.cplusplus.SmartPtr.

The checked pointers are:

  • The return value of createArgument in file clang/utils/TableGen/ClangAttrEmitter.cpp. Although there are a lot of checks in the function, nullptr is still allowed to be returned. As a recursive function it is, I added checks to all the places where the function is called.
  • The local variable Unit in function DWARFLinker::loadClangModule in file llvm/lib/DWARFLinker/DWARFLinker.cpp. If the variable is not set in the loop below its definition, it will trigger a null pointer dereference after the loop.
  • The local variable Index in function ThinLTOCodeGenerator::run in file llvm/lib/LTO/ThinLTOCodeGenerator.cpp. When function ThinLTOCodeGenerator::linkCombinedIndex returns nullptr, the pointer Index will be null and be dereferenced below.
  • The parameter variable Buffer in function InMemoryFileSystem::addFile in file llvm/lib/Support/VirtualFileSystem.cpp. The assertion in this function (assert(!(HardLinkTarget && Buffer))) only checks whether these two parameters can both be non-null. But It can be inferred that both pointers can be null together. A null Buffer pointer can be dereferenced without a check.
  • The return value of function ModuleLazyLoaderCache::operator in file llvm/tools/llvm-link/llvm-link.cpp. According to the bug report of my static analyzer, the std::function variable ModuleLazyLoaderCache::createLazyModule points to function loadFile, which may return nullptr when error. And the pointer is returned as a reference without a check to the return value.
  • The local variable Ret in function MarshallingKindInfo::create in file llvm/utils/TableGen/OptParserEmitter.cpp. If not all MarshallingKind's are handled, variable Ret will be kept as nullptr.

Diff Detail

Unit TestsFailed

TimeTest
430 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

OikawaKirie created this revision.Nov 19 2020, 11:35 PM
OikawaKirie requested review of this revision.Nov 19 2020, 11:35 PM

Is it possible to split these up into separate patches for unrelated code?

clang/utils/TableGen/ClangAttrEmitter.cpp
1346–1353

Can we just add a single assertion here? It looks to me like every caller wants a valid return.

Is it possible to split these up into separate patches for unrelated code?

Since these are reported by one static scan, and these reported places cannot be categorized with others, I choose to submit them in one patch for simplicity and avoiding spam. If it is necessary to separate them one by one, I will close this review and start a new one for each of them.

Or, maybe you are thinking of just separating the patch of clang with llvm? If so, I will start a new review just for the patch of clang and leave the patches of llvm here.

clang/utils/TableGen/ClangAttrEmitter.cpp
1346–1353

Ok, I will add an assertion here (below line 1353) in the new submits, and remove all other assertions I added in this file together with the checks on this pointer after the assertion (line 1355 and 1358).

avl added a comment.Nov 23 2020, 11:48 AM

The change for DWARFLinker is fine. I also think it is better to split the patch up into separate patches.

jansvoboda11 resigned from this revision.Feb 19 2021, 5:18 AM
OikawaKirie abandoned this revision.Feb 22 2021, 10:46 PM

Split to

  • D97251 (clang/utils/TableGen/ClangAttrEmitter.cpp)
  • D97185 (llvm/lib/DWARFLinker/DWARFLinker.cpp)
  • D97254 (llvm/lib/LTO/ThinLTOCodeGenerator.cpp)
  • D97255 (llvm/lib/Support/VirtualFileSystem.cpp)
  • D97258 (llvm/tools/llvm-link/llvm-link.cpp)

And the last one (llvm/utils/TableGen/OptParserEmitter.cpp) has been fixed by others.

Please review the new revisions, and this revision will be closed.

Thanks to all reviewers and sorry for the delay.