This is an archive of the discontinued LLVM Phabricator instance.

[llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref
ClosedPublic

Authored by OikawaKirie on Nov 13 2020, 3:32 AM.

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 in this patch are initialized by Target::createXXX functions. When the creator function pointer is not correctly set, a null pointer will be returned, or the creator function may originally return a null pointer.

Some of them may not make sense as they may be checked before entering the function, but I fixed them all in this patch. I submit this fix because 1) similar checks are found in some other places in the LLVM codebase for the same return value of the function; and, 2) some of the pointers are dereferenced before they are checked, which may definitely trigger a null pointer dereference if the return value is nullptr.

Diff Detail

Event Timeline

OikawaKirie created this revision.Nov 13 2020, 3:32 AM
OikawaKirie requested review of this revision.Nov 13 2020, 3:32 AM

LGTM for the MLIR part

LGTM for llvm-exegesis

LGTM for llvm-ml.

I was reviewing the llvm/lib/* changes and note that the new checking is a mix of assert (debug or assert enabled compiler only) and fatal errors (both debug and release compilers). Is that intended? If these are not likely due to user input issues, then perhaps they should all be assert so that they are compiled out in release compilers?

... the new checking is a mix of assert and fatal errors. Is that intended?

No. The added checks are based on the checks on other calls to the Target::createXXX functions in this file or other related files. If there are any fatal errors nearby (e.g. llvm/lib/LTO/ThinLTOCodeGenerator.cpp:581 vs 569), the check will be a fatal error; and if there are any assertions (e.g. llvm/lib/CodeGen/LLVMTargetMachine.cpp:43,45,52 vs 60) or the calls are never checked (e.g. llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:300), the added check will be an assertion.

If these are not likely due to user input issues, then perhaps they should all be assert so that they are compiled out in release compilers?

Since all these problems are reported by my static analyzer, I do not really know whether these checked pointers will actually be null when the code is executed. And I did not try to dynamically execute the program to check the problems either. But chances are that if the creator callbacks are not properly set or the called creator functions returns nullptr, the problem will happen. In my opinion, these problems may only happen during development. Therefore, I believe asserts can be sufficient to diagnose the problems.

If you think it would be better to use assertions instead of fatal errors, I will make an update on all llvm/lib/xxx files (or maybe all files). But before that, I'd prefer waiting for the replies from other reviewers on the remaining parts of the patch and making an update for all the suggestions.

Thank you.

... the new checking is a mix of assert and fatal errors. Is that intended?

No. The added checks are based on the checks on other calls to the Target::createXXX functions in this file or other related files. If there are any fatal errors nearby (e.g. llvm/lib/LTO/ThinLTOCodeGenerator.cpp:581 vs 569), the check will be a fatal error; and if there are any assertions (e.g. llvm/lib/CodeGen/LLVMTargetMachine.cpp:43,45,52 vs 60) or the calls are never checked (e.g. llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:300), the added check will be an assertion.

If these are not likely due to user input issues, then perhaps they should all be assert so that they are compiled out in release compilers?

Since all these problems are reported by my static analyzer, I do not really know whether these checked pointers will actually be null when the code is executed. And I did not try to dynamically execute the program to check the problems either. But chances are that if the creator callbacks are not properly set or the called creator functions returns nullptr, the problem will happen. In my opinion, these problems may only happen during development. Therefore, I believe asserts can be sufficient to diagnose the problems.

If you think it would be better to use assertions instead of fatal errors, I will make an update on all llvm/lib/xxx files (or maybe all files). But before that, I'd prefer waiting for the replies from other reviewers on the remaining parts of the patch and making an update for all the suggestions.

Sure that sounds good. (Otherwise the llvm/lib parts LGTM)

LGTM for llvm-mca.

MaskRay accepted this revision.Nov 19 2020, 10:16 AM

Seems that sufficient approvals have been received.

This revision is now accepted and ready to land.Nov 19 2020, 10:16 AM
  1. Replace fatal errors with assertions.

In D91410#2400018, @tejohnson wrote:

If these are not likely due to user input issues, then perhaps they should all be assert so that they are compiled out in release compilers?

  1. Fix clang-format errors in llvm/tools/llvm-objdump/MachODump.cpp
jpienaar accepted this revision.Nov 21 2020, 6:26 AM
jpienaar added a subscriber: jpienaar.

LG MLIR changes