[analyzer] StdVariantChecker: fix crashes and incorrect retrieval of template arguments

[guillem-bartrina-sonarsource]

Although very unusual, the SVal of the argument is not checked for UnknownVal, so we may get a null pointer dereference.

In addition, the template arguments of the variant are retrieved incorrectly when type aliases are involved, causing crashes and FPs/FNs.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (guillem-bartrina-sonarsource)

Changes

Although very unusual, the SVal of the argument is not checked for UnknownVal, so we may get a null pointer dereference.

---

Full diff: https://github.com/llvm/llvm-project/pull/167341.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp (+6-4)
• (modified) clang/test/Analysis/std-variant-checker.cpp (+12-1)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
index db8bbee8761d5..805f64f4804cf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
@@ -219,10 +219,12 @@ class StdVariantChecker : public Checker<eval::Call, check::RegionChanges> {
   bool handleStdGetCall(const CallEvent &Call, CheckerContext &C) const {
     ProgramStateRef State = C.getState();
 
-    const auto &ArgType = Call.getArgSVal(0)
-                              .getType(C.getASTContext())
-                              ->getPointeeType()
-                              .getTypePtr();
+    SVal ArgSVal = Call.getArgSVal(0);
+    if (ArgSVal.isUnknown())
+      return false;
+
+    const auto &ArgType =
+        ArgSVal.getType(C.getASTContext())->getPointeeType().getTypePtr();
     // We have to make sure that the argument is an std::variant.
     // There is another std::get with std::pair argument
     if (!isStdVariant(ArgType))
diff --git a/clang/test/Analysis/std-variant-checker.cpp b/clang/test/Analysis/std-variant-checker.cpp
index 7f136c06b19cc..fbb69327e1de5 100644
--- a/clang/test/Analysis/std-variant-checker.cpp
+++ b/clang/test/Analysis/std-variant-checker.cpp
@@ -355,4 +355,15 @@ void nonInlineFunctionCallPtr() {
   char c = std::get<char> (v); // no-warning
   (void)a;
   (void)c;
-}
\ No newline at end of file
+}
+
+// ----------------------------------------------------------------------------//
+// Misc
+// ----------------------------------------------------------------------------//
+
+using uintptr_t = unsigned long long;
+
+void unknownVal() {
+  // force the argument to be UnknownVal
+  (void)std::get<int>(*(std::variant<int, float>*)(uintptr_t)3.14f); // no crash
+}

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

Author: None (guillem-bartrina-sonarsource)

Changes

Although very unusual, the SVal of the argument is not checked for UnknownVal, so we may get a null pointer dereference.

---

Full diff: https://github.com/llvm/llvm-project/pull/167341.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp (+6-4)
• (modified) clang/test/Analysis/std-variant-checker.cpp (+12-1)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
index db8bbee8761d5..805f64f4804cf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
@@ -219,10 +219,12 @@ class StdVariantChecker : public Checker<eval::Call, check::RegionChanges> {
   bool handleStdGetCall(const CallEvent &Call, CheckerContext &C) const {
     ProgramStateRef State = C.getState();
 
-    const auto &ArgType = Call.getArgSVal(0)
-                              .getType(C.getASTContext())
-                              ->getPointeeType()
-                              .getTypePtr();
+    SVal ArgSVal = Call.getArgSVal(0);
+    if (ArgSVal.isUnknown())
+      return false;
+
+    const auto &ArgType =
+        ArgSVal.getType(C.getASTContext())->getPointeeType().getTypePtr();
     // We have to make sure that the argument is an std::variant.
     // There is another std::get with std::pair argument
     if (!isStdVariant(ArgType))
diff --git a/clang/test/Analysis/std-variant-checker.cpp b/clang/test/Analysis/std-variant-checker.cpp
index 7f136c06b19cc..fbb69327e1de5 100644
--- a/clang/test/Analysis/std-variant-checker.cpp
+++ b/clang/test/Analysis/std-variant-checker.cpp
@@ -355,4 +355,15 @@ void nonInlineFunctionCallPtr() {
   char c = std::get<char> (v); // no-warning
   (void)a;
   (void)c;
-}
\ No newline at end of file
+}
+
+// ----------------------------------------------------------------------------//
+// Misc
+// ----------------------------------------------------------------------------//
+
+using uintptr_t = unsigned long long;
+
+void unknownVal() {
+  // force the argument to be UnknownVal
+  (void)std::get<int>(*(std::variant<int, float>*)(uintptr_t)3.14f); // no crash
+}

[guillem-bartrina-sonarsource]

@steakhal

[steakhal]

I resign from reviews for the upcoming 2 weeks. Thank you

[spaits]

LGTM! Thanks!

[NagyDonat]

Thanks for the commit, crash fixes are always welcome :) LGTM, but let's wait a few days to see if @spaits (who authored this checker) has anything to add. (EDIT: Oh, I see that he reviewed the commit just a few minutes before my review. Feel free to merge the commit.)

[guillem-bartrina-sonarsource]

Thanks @NagyDonat and @spaits for the comments. I don't have permission to merge this pull request, so I'll leave it to you. Have a nice day!

[steakhal]

LGTM, thanks for the reviews and the patch.