Page MenuHomePhabricator

[analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move.
ClosedPublic

Authored by NoQ on Dec 6 2018, 3:02 PM.

Details

Summary

Calling operator*() or operator->() on a null STL smart pointer is undefined behavior.

Smart pointers are specified to become null after being moved from. So we can't warn on arbitrary method calls, but these two operators definitely make no sense.

The new bug is fatal, because it's UB unlike other use-after-move bugs.

Generally, we should also make a checker for dereferencing smart pointers that are known to be null, regardless of why they are null. Probably these checkers could interact somehow. But this case is kinda handy and is available to us almost for free.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Dec 6 2018, 3:02 PM
MTC accepted this revision.Dec 6 2018, 10:52 PM
MTC added a subscriber: MTC.
MTC added inline comments.
lib/StaticAnalyzer/Checkers/MoveChecker.cpp
89–90

Outdated TODO?

This revision is now accepted and ready to land.Dec 6 2018, 10:52 PM

Hm. I wonder if it would also make sense to model e.g. the get method to return nullptr for moved from smart ptrs. This could help null dereference checker and also aid false path prunning.

NoQ updated this revision to Diff 177307.Dec 7 2018, 2:01 PM
NoQ marked an inline comment as done.

Fxd!

NoQ updated this revision to Diff 177332.Dec 7 2018, 2:53 PM

Fix typo.

MTC added a comment.Dec 9 2018, 6:45 AM

Hm. I wonder if it would also make sense to model e.g. the get method to return nullptr for moved from smart ptrs. This could help null dereference checker and also aid false path prunning.

Great idea!

IIRC, we haven't model smart-pointer yet. There are no warnings fired in the simple code below.

int foo() {
    auto ptr = std::make_unique<int>(0);
    ptr = nullptr;
    return *ptr;              // <--------- Should emit a warning here
}

If we want to model the get() method, not only the move action but also the assignment should be considered. std::unique_ptr may be fine, but is the reference count stuff too complex for std::shared_ptr to get the information available for analysis? Can the lifetime analysis cover the smart pointers?

int unique_ptr_bad() {
    auto ptr = std::make_unique<int>(0);
    ptr = nullptr;
    int *raw_p = ptr.get();   // <--------- 'raw_p' is definitely null here
    return *raw_p;              // <--------- Should emit a warning here
}

Related dev-mail about smart pointer checkers, see http://lists.llvm.org/pipermail/cfe-dev/2012-January/019446.html

NoQ added a comment.Dec 13 2018, 3:34 PM

Hm. I wonder if it would also make sense to model e.g. the get method to return nullptr for moved from smart ptrs. This could help null dereference checker and also aid false path prunning.

Apart from "yeah, totally, and we should also model the whole standard library", i'm also a bit uncomfy to rely on this generic checker for something as specific as evalCall(). This whole patch should probably be split out into a separate checker that shares state with the move checker but is dedicated to smart pointers specifically. I'm pushing for this before coming up with generic standard library modeling because i was asked to do this specific check. Not sure how much should i push it though. Null dereferences via .get() are definitely a tasty addition.

Another random thought: should we make a mechanism through which checkers could send events to each other? Imagine move checker notifying other checkers that an object is being moved, and checkers subscribed to such custom event could immediately react accordingly. Kinda checker-defined checker callbacks.

NoQ updated this revision to Diff 178273.Dec 14 2018, 1:27 PM

Rebase.

NoQ added inline comments.Dec 14 2018, 4:16 PM
test/Analysis/use-after-move.cpp
239

Ugh. Why are these diagnostic pieces incorrect? We're not jumping to the end of the function yet. At the very least, we should evaluate the next A a;.

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Dec 14 2018, 7:03 PM

Reverted in rC349233 - fails on a Windows buildbot (http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/2642/steps/annotate/logs/stdio).

Surprisingly, the next build is fine, and the build after it is broken again. Probably some UB.

******************** TEST 'Clang :: Analysis/use-after-move.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\b\slave\clang-x64-windows-msvc\build\build\stage1\bin\clang.EXE -cc1 -internal-isystem c:\b\slave\clang-x64-windows-msvc\build\build\stage1\lib\clang\8.0.0\include -nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=alpha.cplusplus.Move -verify C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false  -analyzer-config exploration_strategy=unexplored_first_queue  -analyzer-checker debug.ExprInspection
: 'RUN: at line 5';   c:\b\slave\clang-x64-windows-msvc\build\build\stage1\bin\clang.EXE -cc1 -internal-isystem c:\b\slave\clang-x64-windows-msvc\build\build\stage1\lib\clang\8.0.0\include -nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=alpha.cplusplus.Move -verify C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false  -analyzer-config exploration_strategy=dfs -DDFS=1  -analyzer-checker debug.ExprInspection
: 'RUN: at line 9';   c:\b\slave\clang-x64-windows-msvc\build\build\stage1\bin\clang.EXE -cc1 -internal-isystem c:\b\slave\clang-x64-windows-msvc\build\build\stage1\lib\clang\8.0.0\include -nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=alpha.cplusplus.Move -verify C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false  -analyzer-config exploration_strategy=unexplored_first_queue  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE  -analyzer-checker debug.ExprInspection
: 'RUN: at line 14';   c:\b\slave\clang-x64-windows-msvc\build\build\stage1\bin\clang.EXE -cc1 -internal-isystem c:\b\slave\clang-x64-windows-msvc\build\build\stage1\lib\clang\8.0.0\include -nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=alpha.cplusplus.Move -verify C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false  -analyzer-config exploration_strategy=dfs -DDFS=1  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE  -analyzer-checker debug.ExprInspection
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\b\slave\clang-x64-windows-msvc\build\build\stage1\bin\clang.EXE" "-cc1" "-internal-isystem" "c:\b\slave\clang-x64-windows-msvc\build\build\stage1\lib\clang\8.0.0\include" "-nostdsysteminc" "-analyze" "-analyzer-constraints=range" "-analyzer-checker=alpha.cplusplus.Move" "-verify" "C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp" "-std=c++11" "-analyzer-output=text" "-analyzer-config" "eagerly-assume=false" "-analyzer-config" "exploration_strategy=unexplored_first_queue" "-analyzer-checker" "debug.ExprInspection"
# command stderr:
error: 'warning' diagnostics expected but not seen: 

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 823 (directive at C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp:825): Dereference of null smart pointer 'P' of type 'std::unique_ptr'

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 851: Dereference of null smart pointer 'P' of type 'std::unique_ptr'

error: 'warning' diagnostics seen but not expected: 

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 831: REACHABLE

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 851: Method called on moved-from object 'P'

error: 'note' diagnostics expected but not seen: 

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 812 (directive at C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp:826): Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 823 (directive at C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp:827): Dereference of null smart pointer 'P' of type 'std::unique_ptr'

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 844: Object 'P' is moved

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 850: Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 851 (directive at C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp:852): Dereference of null smart pointer 'P' of type 'std::unique_ptr'

error: 'note' diagnostics seen but not expected: 

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 831: REACHABLE

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 844: 

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 850: 

  File C:\b\slave\clang-x64-windows-msvc\build\llvm.src\tools\clang\test\Analysis\use-after-move.cpp Line 851: Method called on moved-from object 'P'

13 errors generated.


error: command failed with exit status: 1
NoQ reopened this revision.Dec 14 2018, 7:32 PM
This revision is now accepted and ready to land.Dec 14 2018, 7:32 PM
This revision was automatically updated to reflect the committed changes.