[clang][dataflow] Add support for disabling warnings on smart pointers.

[ymandel]

Summary

This patch provides the user with the ability to disable all checked of accesses
to optionals that are the pointees of smart pointers. Since smart pointers are
not modeled (yet), the system cannot distinguish safe from unsafe accesses to
optionals through smart pointers. This results in false positives whenever
optionals are used through smart pointers. The patch gives the user the choice
of ignoring all positivess in these cases.

[xazax.hun]

Are smart pointers special? I would expect to see similar problems with containers (or even a nested optional). I wonder whether an allowlist instead of a denylist approach is better here. E.g., instead of disabling the modeling for smart pointers, we could enable it for cases that we actually support (or alternatively, we could add a confidence value to the unsafe access). Usually, these checks are pretty robust when we deal with objects on the stack of the analyzed function (locals, parameters), but it is really hard to reason about objects from the outside (e.g., when a reference to an object is acquired from a container or smart pointer) unless we have explicit modeling for the APIs. The confidence approach might be useful as we are unlikely to cover all the custom smart pointers the users have.

[ymandel]

In D122143#3396615, @xazax.hun wrote:

Are smart pointers special? I would expect to see similar problems with containers (or even a nested optional).

No, they're not. You're quite right -- containers in general are a problem. We happen to have support (landing soon) for nested optionals, but the general point still holds.

I wonder whether an allowlist instead of a denylist approach is better here. E.g., instead of disabling the modeling for smart pointers, we could enable it for cases that we actually support.

I like this idea, although we'll need to work out how to describe the set of things we track effectively. My argument against would be that it moves to "unsafe" as the default. But, given that we know we'll *always* report (right or wrong) in these situations, the safety argument isn't tht strong, because we're simply not adding anything of value in our reports -- users could just as well be told to always double check optionals in containers, etc.

So, do you mean to add a FIXME to move to allowlist, or do you mean to hold off until we've switched? I have a short-term interest in getting this through for a particular usecase, but I understand if you feel it just not a good idea. Regardless, I'm going to get started exploring an allowlist approach.

(or alternatively, we could add a confidence value to the unsafe access). Usually, these checks are pretty robust when we deal with objects on the stack of the analyzed function (locals, parameters), but it is really hard to reason about objects from the outside (e.g., when a reference to an object is acquired from a container or smart pointer) unless we have explicit modeling for the APIs. The confidence approach might be useful as we are unlikely to cover all the custom smart pointers the users have.

This idea sounds useful, but I'm not really sure how it would play out. I suppose we'd then let the user set a confidence level for diagnostics (like logging level?). Regardless, I think for a first attempt, I'd rather go with binary yes/no. But interested in exploring this approach.

[xazax.hun]

In D122143#3396695, @ymandel wrote:

So, do you mean to add a FIXME to move to allowlist, or do you mean to hold off until we've switched? I have a short-term interest in getting this through for a particular usecase, but I understand if you feel it just not a good idea. Regardless, I'm going to get started exploring an allowlist approach.

I see the precedent of FIXMEs getting followed up in this effort, so I'm fine with a FIXME for now :)

(or alternatively, we could add a confidence value to the unsafe access). Usually, these checks are pretty robust when we deal with objects on the stack of the analyzed function (locals, parameters), but it is really hard to reason about objects from the outside (e.g., when a reference to an object is acquired from a container or smart pointer) unless we have explicit modeling for the APIs. The confidence approach might be useful as we are unlikely to cover all the custom smart pointers the users have.

This idea sounds useful, but I'm not really sure how it would play out. I suppose we'd then let the user set a confidence level for diagnostics (like logging level?). Regardless, I think for a first attempt, I'd rather go with binary yes/no. But interested in exploring this approach.

Some tools I worked with will have separate warnings for high and low confidence reports, so users can chose to enable both or only one of them. It can be useful when different users have different expectations, e.g., some thinks of it like verification of a safety property and willing to make changes to make the tool happy while others would think of it as a bug finding tool and would be annoyed by false positives.

[xazax.hun]

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

63

Maybe removing the duplication with something like: auto Exception = Ignorable? expr() : expr(unless(...)) and use it in the matcher?

349

It is probably rarely used, but one could use std::unique_ptr with an array of std::optionals and use operator[] to access the optional.

[ymandel]

Addressed comments.

[ymandel]

add fixme

[ymandel]

In D122143#3396729, @xazax.hun wrote:

In D122143#3396695, @ymandel wrote:

So, do you mean to add a FIXME to move to allowlist, or do you mean to hold off until we've switched? I have a short-term interest in getting this through for a particular usecase, but I understand if you feel it just not a good idea. Regardless, I'm going to get started exploring an allowlist approach.

I see the precedent of FIXMEs getting followed up in this effort, so I'm fine with a FIXME for now :)

Thanks for the confidence. FIXME added.

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

349

I added a note to the option itself and also changed its name to be more specific.

[sgatev]

clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h

31

Can you clarify what "smart pointer" refers to? It's not only standard types like unique_ptr, shared_ptr, and weak_ptr, right?

32

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

1932–1936

Let's add this to a memory.h header.

1938

Why Member? I suggest either using one of the generic names that we use above or maybe something more descriptive.

[ymandel]

Addressed comments

[ymandel]

clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h

31

I've rewritten it to specify the operators directly.

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

1932–1936

I specifically wanted a locally-defined type. Notice that it's not in std. I've renamed to generic "smart_ptr" to emphasize that. WDYT?

1938

Went with generic. I think Member was from copy pasting...

clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h

 #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/MatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/SourceLocationsLattice.h"
 
 namespace clang {
 namespace dataflow {
 
+// FIXME: Explore using an allowlist-approach, where constructs supported by the
+// analysis are always enabled and additional constructs are enabled through the
+// `Options`.
+struct UncheckedOptionalAccessModelOptions {
+  /// Ignore optionals reachable through overloaded `operator*` or `operator->`
+  /// (other than those of the optional type itself). The analysis does not
+  /// equate the results of such calls, so it can't identify when their results
+  /// are used safely (across calls), resulting in false positives in all such
+  /// cases. Note: this option does not cover access through `operator[]`.
+  bool IgnoreSmartPointerDereference = false;
+};
+
 /// Dataflow analysis that discovers unsafe accesses of optional values and
 /// adds the respective source locations to the lattice.
 ///
 /// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
 ///
 /// FIXME: Consider separating the models from the unchecked access analysis.
 class UncheckedOptionalAccessModel
     : public DataflowAnalysis<UncheckedOptionalAccessModel,
                               SourceLocationsLattice> {
 public:
-  explicit UncheckedOptionalAccessModel(ASTContext &AstContext);
+  UncheckedOptionalAccessModel(
+      ASTContext &AstContext, UncheckedOptionalAccessModelOptions Options = {});
 
   static SourceLocationsLattice initialElement() {
     return SourceLocationsLattice();
   }
 
   void transfer(const Stmt *Stmt, SourceLocationsLattice &State,
                 Environment &Env);
 
 private:
   MatchSwitch<TransferState<SourceLocationsLattice>> TransferMatchSwitch;
 };
 
 } // namespace dataflow
 } // namespace clang
 
 #endif // CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

return classTemplateSpecializationDecl(
       anyOf(hasName("std::optional"), hasName("std::__optional_storage_base"),
             hasName("__optional_destruct_base"), hasName("absl::optional"),
             hasName("base::Optional")),
       hasTemplateArgument(0, refersToType(type().bind("T"))));
 }
 
 auto hasOptionalType() { return hasType(optionalClass()); }
 
-auto isOptionalMemberCallWithName(llvm::StringRef MemberName) {
+auto isOptionalMemberCallWithName(
+    llvm::StringRef MemberName,
+    llvm::Optional<StatementMatcher> Ignorable = llvm::None) {
+  auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
+                                    : cxxThisExpr());
   return cxxMemberCallExpr(
-      on(expr(unless(cxxThisExpr()))),
+      on(expr(Exception)),
       callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass()))));
 }
 
-auto isOptionalOperatorCallWithName(llvm::StringRef OperatorName) {
-  return cxxOperatorCallExpr(hasOverloadedOperatorName(OperatorName),
-                             callee(cxxMethodDecl(ofClass(optionalClass()))));
+auto isOptionalOperatorCallWithName(
+    llvm::StringRef operator_name,
+    llvm::Optional<StatementMatcher> Ignorable = llvm::None) {
+  return cxxOperatorCallExpr(
+      hasOverloadedOperatorName(operator_name),
+      callee(cxxMethodDecl(ofClass(optionalClass()))),
+      Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
 }
 
 auto isMakeOptionalCall() {
   return callExpr(
       callee(functionDecl(hasAnyName(
           "std::make_optional", "base::make_optional", "absl::make_optional"))),
       hasOptionalType());
 }
void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &,
 
   auto *OptionalLoc2 =
       State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference);
   assert(OptionalLoc2 != nullptr);
 
   transferSwap(*OptionalLoc1, *OptionalLoc2, State);
 }
 
-auto buildTransferMatchSwitch() {
+llvm::Optional<StatementMatcher>
+ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
+  if (Options.IgnoreSmartPointerDereference)
+    return memberExpr(hasObjectExpression(ignoringParenImpCasts(
+        cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("->"),
+                                  hasOverloadedOperatorName("*")),
+                            unless(hasArgument(0, expr(hasOptionalType())))))));
+  return llvm::None;
+}
+
+auto buildTransferMatchSwitch(
+    const UncheckedOptionalAccessModelOptions &Options) {
   // FIXME: Evaluate the efficiency of matchers. If using matchers results in a
   // lot of duplicated work (e.g. string comparisons), consider providing APIs
   // that avoid it through memoization.
+  auto IgnorableOptional = ignorableOptional(Options);
   return MatchSwitchBuilder<LatticeTransferState>()
       // Attach a symbolic "has_value" state to optional values that we see for
       // the first time.
       .CaseOf<Expr>(expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()),
                     initializeOptionalReference)
 
       // make_optional
       .CaseOf<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall)
return MatchSwitchBuilder<LatticeTransferState>()
       // optional::operator=
       .CaseOf<CXXOperatorCallExpr>(isOptionalValueOrConversionAssignment(),
                                    transferValueOrConversionAssignment)
       .CaseOf<CXXOperatorCallExpr>(isOptionalNulloptAssignment(),
                                    transferNulloptAssignment)
 
       // optional::value
       .CaseOf<CXXMemberCallExpr>(
-          isOptionalMemberCallWithName("value"),
+          isOptionalMemberCallWithName("value", IgnorableOptional),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
           })
 
       // optional::operator*, optional::operator->
-      .CaseOf<CallExpr>(expr(anyOf(isOptionalOperatorCallWithName("*"),
-                                   isOptionalOperatorCallWithName("->"))),
-                        [](const CallExpr *E, const MatchFinder::MatchResult &,
-                           LatticeTransferState &State) {
-                          transferUnwrapCall(E, E->getArg(0), State);
-                        })
+      .CaseOf<CallExpr>(
+          expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional),
+                     isOptionalOperatorCallWithName("->", IgnorableOptional))),
 
       // optional::has_value
       .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("has_value"),
                                  transferOptionalHasValueCall)
 
       // optional::operator bool
       .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("operator bool"),
                                  transferOptionalHasValueCall)
return MatchSwitchBuilder<LatticeTransferState>()
       // std::swap
       .CaseOf<CallExpr>(isStdSwapCall(), transferStdSwapCall)
 
       .Build();
 }
 
 } // namespace
 
-UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx)
+UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
+    ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
     : DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>(
           Ctx),
-      TransferMatchSwitch(buildTransferMatchSwitch()) {}
+      TransferMatchSwitch(buildTransferMatchSwitch(Options)) {}
 
 void UncheckedOptionalAccessModel::transfer(const Stmt *S,
                                             SourceLocationsLattice &L,
                                             Environment &Env) {
   LatticeTransferState State(L, Env);
   TransferMatchSwitch(*S, getASTContext(), State);
 }
 
 } // namespace dataflow
 } // namespace clang

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Headers.emplace_back("unchecked_optional_access_test.h", R"(
       template <typename T>
       T Make();
     )");
     const tooling::FileContentMappings FileContents(Headers.begin(),
                                                     Headers.end());
     llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
         SourceCode, FuncMatcher,
         [](ASTContext &Ctx, Environment &) {
-          return UncheckedOptionalAccessModel(Ctx);
+          return UncheckedOptionalAccessModel(
+              Ctx, UncheckedOptionalAccessModelOptions{
+                       /*IgnoreSmartPointerDereference=*/true});
         },
         [&MatchesLatticeChecks](
             llvm::ArrayRef<std::pair<
                 std::string, DataflowAnalysisState<SourceLocationsLattice>>>
                 CheckToLatticeMap,
             ASTContext &Ctx) {
           // FIXME: Consider using a matcher instead of translating
           // `CheckToLatticeMap` to `CheckToStringifiedLatticeMap`.

 
 TEST_P(UncheckedOptionalAccessTest, OptionalSwap) {
   ExpectLatticeChecksFor(
       R"(
     #include "unchecked_optional_access_test.h"
 
     void target() {
       $ns::$optional<int> opt1 = $ns::nullopt;
       $ns::$optional<int> opt2 = 3;
 
       opt1.swap(opt2);
 
       opt1.value();
       /*[[check-1]]*/
 
       opt2.value();
       /*[[check-2]]*/
     }
   )",
       UnorderedElementsAre(Pair("check-1", "safe"),
                            Pair("check-2", "unsafe: input.cc:13:7")));
 
   ExpectLatticeChecksFor(
void target() {
       opt2.value();
       /*[[check-4]]*/
     }
   )",
       UnorderedElementsAre(Pair("check-3", "safe"),
                            Pair("check-4", "unsafe: input.cc:13:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, UniquePtrToStructWithOptionalField) {
+  // We suppress diagnostics for values reachable from smart pointers (other
+  // than `optional` itself).
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    template <typename T>
+    struct smart_ptr {
+      T& operator*() &;
+      T* operator->();
+    };
+
+    struct Foo {
+      $ns::$optional<int> opt;
+    };
+
+    void target() {
+      smart_ptr<Foo> foo;
+      *foo->opt;
+      /*[[check-1]]*/
+      *(*foo).opt;
+      /*[[check-2]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
 // - invalidation (passing optional by non-const reference/pointer)
 // - `value_or(nullptr) != nullptr`, `value_or(0) != 0`, `value_or("").empty()`
 // - nested `optional` values