[analyzer] Fix crash in BasicValueFactory.cpp with __int128_t integers

[vabridgers]

This change avoids a crash in BasicValueFactory by checking the bit width of an APSInt to avoid calling getZExtValue if greater than 64-bits. This was caught by our internal, randomized test generator.

Clang invocation
clang -cc1 -analyzer-checker=optin.portability.UnixAPI case.c

/llvm/include/llvm/ADT/APInt.h:1488:
uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64
&& "Too many bits for uint64_t"' failed.
...

#9

llvm::APInt::getZExtValue() const
/llvm/include/llvm/ADT/APInt.h:1488:5
clang::BinaryOperatorKind, llvm::APSInt const&, llvm::APSInt const&)
/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:307:37
llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::BinaryOperatorKind, clang::ento::NonLoc, clang::ento::NonLoc,
clang::QualType)
/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:531:31
llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::BinaryOperatorKind, clang::ento::SVal, clang::ento::SVal,
clang::QualType)
/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:532:26
...

[llvmbot]

@llvm/pr-subscribers-clang

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

Changes

This change avoids a crash in BasicValueFactory by checking the bit width of an APSInt to avoid calling getZExtValue if greater than 64-bits. This was caught by our internal, randomized test generator.

Clang invocation
clang -cc1 -analyzer-checker=optin.portability.UnixAPI case.c

<src-root>/llvm/include/llvm/ADT/APInt.h:1488:
uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64
&& "Too many bits for uint64_t"' failed.
...

#9 <address> llvm::APInt::getZExtValue() const
<src-root>/llvm/include/llvm/ADT/APInt.h:1488:5
clang::BinaryOperatorKind, llvm::APSInt const&, llvm::APSInt const&)
<src-root>/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:307:37
llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::BinaryOperatorKind, clang::ento::NonLoc, clang::ento::NonLoc,
clang::QualType)
<src-root>/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:531:31
llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::BinaryOperatorKind, clang::ento::SVal, clang::ento::SVal,
clang::QualType)
<src-root>/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:532:26
...

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp (+6)
• (added) clang/test/Analysis/int128-nocrash.c (+10)

diff --git a/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp b/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
index 5924f6a671c2ac1..351735cdf42dc8f 100644
--- a/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -275,6 +275,9 @@ BasicValueFactory::evalAPSInt(BinaryOperator::Opcode Op,
       if (V2.isSigned() && V2.isNegative())
         return nullptr;
 
+      if (V2.getBitWidth() > 64)
+        return nullptr;
+
       uint64_t Amt = V2.getZExtValue();
 
       if (Amt >= V1.getBitWidth())
@@ -298,6 +301,9 @@ BasicValueFactory::evalAPSInt(BinaryOperator::Opcode Op,
       if (V2.isSigned() && V2.isNegative())
         return nullptr;
 
+      if (V2.getBitWidth() > 64)
+        return nullptr;
+
       uint64_t Amt = V2.getZExtValue();
 
       if (Amt >= V1.getBitWidth())
diff --git a/clang/test/Analysis/int128-nocrash.c b/clang/test/Analysis/int128-nocrash.c
new file mode 100644
index 000000000000000..0e9d2322080b8b8
--- /dev/null
+++ b/clang/test/Analysis/int128-nocrash.c
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=optin.portability.UnixAPI \
+// RUN:    -triple x86_64-pc-linux-gnu -x c %s
+
+// Don't crash!
+// expected-no-diagnostics
+const __int128_t a = ( ((__int128_t)1) << 64 | 1);
+
+void b() { 
+  2 >> a; 
+}

[NagyDonat]

Good commit! I was suspicious at first because getBitWidth() tends to be problematic, but I couldn't find any concrete case where the new code behaves incorrectly.

[vabridgers]

I'll resolve the comments in an update. Thanks for the comments!

[steakhal]

I support changes like this. However, I think we should prefer reusing existing test files to creating more and more new ones. That has one benefit to me, one can see multiple cases of the topic on one screen without jumping around to open multiple.
But that's all. I don't think I'll participate in the review though. They are piling up already.

[vabridgers]

I support changes like this. However, I think we should prefer reusing existing test files to creating more and more new ones. That has one benefit to me, one can see multiple cases of the topic on one screen without jumping around to open multiple. But that's all. I don't think I'll participate in the review though. They are piling up already.

Thanks @steakhal, I'll look at including this test into an existing case. Best

[vabridgers]

@steakhal, I looked at the existing test cases, but cannot find an existing test where this case would easily fit. I'm happy to follow a specific suggestion about where or how to reformat or rename this test.

For now, I'll assume this current approach is ok until I see some concrete suggestions.

Thanks

[vabridgers]

Hey guys, is it ok to merge this fix? Thanks

[NagyDonat]

It's simple and clean, let's just merge it :)

[AaronBallman]

Please be sure to add a release note about the fix, too!

[vabridgers]

I'll wait for an ok from @AaronBallman before merging. Thank you!

[AaronBallman]

Assuming precommit CI comes back green, the changes LGTM, thank you!