[clang] Fix crash in __builtin_strncmp and other related builtin functions

[shafik]

Summary

The implementation of __builtin_strncmp and other related builtins function use getExtValue() to evaluate the size argument. This can cause a crash when the value does not fit into an int64_t value, which is can be expected since the type of the argument is size_t.

The fix is to switch to using getZExtValue().

This fixes:

https://github.com/llvm/llvm-project/issues/64876

[erichkeane]

Are there any Sema tests we can add to show that we warn/diagnose/SOMETHING on these? If someone passes a negative size, we should probably at least do the warning that it was converted/truncated.

clang/lib/AST/ExprConstant.cpp

9357

Context missing throughout

[shafik]

• Diff w/ context

[shafik]

-Updated values used in test

[shafik]

In D158557#4608262, @erichkeane wrote:

Are there any Sema tests we can add to show that we warn/diagnose/SOMETHING on these? If someone passes a negative size, we should probably at least do the warning that it was converted/truncated.

The arguments have type size_t so using -Wsign-conversion would warn for the previous value but that is not really the point of the fix, which is why I updated the value to clarify.

[aaron.ballman]

Precommit CI found issues that should be addressed, but otherwise the changes LGTM. You should add a release note for the fix, though!

[erichkeane]

I think there is value to ensuring we diagnose the negative-to-size_t conversion here, but I also think there is value to a CodeGen test to ensure we emit the correct value.

[shafik]

• Silence -Wconstant-conversion warning in test

[erichkeane]

Still think a codegen test for this example is VERY valuable here.

clang/test/SemaCXX/constexpr-string.cpp

681

rather than suppress these, it makes sense to me to just mark them expected-warning.

[shafik]

clang/test/SemaCXX/constexpr-string.cpp

681

It depends on the size of size_t whether I get this diagnostic or not.

[aaron.ballman]

clang/test/SemaCXX/constexpr-string.cpp

681

Given that the point to this test isn't to test constant conversion behavior but is testing boundary cases where you'll get those diagnostics, I think suppressing the warnings is fine.

[shafik]

• Add codegen test

[aaron.ballman]

LGTM but the changes need to be landed with a release note.

[erichkeane]

I suggest doing the -511LL value from the original bug report in the tests as well, else, fine to me.

[erichkeane]

Ah, wait, still no release note!

[erichkeane]

I see aaron mentioned the release note and is ok with it happening on commit, so I'll undo my 'needs revision'.

[shafik]

• Extended constexpr-string.cpp test
• Added release notes

[erichkeane]

Still LGTM.

clang/docs/ReleaseNotes.rst

- Fixed an issue where accesses to the local variables of a coroutine during
   object itself.
   (`#56301 <https://github.com/llvm/llvm-project/issues/56301>`_)
   The current solution may bring performance regressions if the awaiters have
   non-static data members. See
   `#64945 <https://github.com/llvm/llvm-project/issues/64945>`_ for details.
 - Clang now prints unnamed members in diagnostic messages instead of giving an
   empty ''. Fixes
   (`#63759 <https://github.com/llvm/llvm-project/issues/63759>`_)
+- Fix crash in __builtin_strncmp and related builtins when the size value
+  exceeded the maximum value representable by int64_t. Fixes
+  (`#64876 <https://github.com/llvm/llvm-project/issues/64876>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/AST/ExprConstant.cpp

bool PointerExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
   case Builtin::BI__builtin_char_memchr:
   case Builtin::BI__builtin_wmemchr: {
     if (!Visit(E->getArg(0)))
       return false;
     APSInt Desired;
     if (!EvaluateInteger(E->getArg(1), Desired, Info))
       return false;
     uint64_t MaxLength = uint64_t(-1);
     if (BuiltinOp != Builtin::BIstrchr &&
         BuiltinOp != Builtin::BIwcschr &&
         BuiltinOp != Builtin::BI__builtin_strchr &&
         BuiltinOp != Builtin::BI__builtin_wcschr) {
       APSInt N;
       if (!EvaluateInteger(E->getArg(2), N, Info))
         return false;
-      MaxLength = N.getExtValue();
+      MaxLength = N.getZExtValue();
     }
     // We cannot find the value if there are no candidates to match against.
     if (MaxLength == 0u)
       return ZeroInitialization(E);
     if (!Result.checkNullPointerForFoldAccess(Info, E, AK_Read) ||
         Result.Designator.Invalid)
       return false;
     QualType CharTy = Result.Designator.getType(Info.Ctx);
case Builtin::BI__builtin_wmemcmp: {
     uint64_t MaxLength = uint64_t(-1);
     if (BuiltinOp != Builtin::BIstrcmp &&
         BuiltinOp != Builtin::BIwcscmp &&
         BuiltinOp != Builtin::BI__builtin_strcmp &&
         BuiltinOp != Builtin::BI__builtin_wcscmp) {
       APSInt N;
       if (!EvaluateInteger(E->getArg(2), N, Info))
         return false;
-      MaxLength = N.getExtValue();
+      MaxLength = N.getZExtValue();
     }
 
     // Empty substrings compare equal by definition.
     if (MaxLength == 0u)
       return Success(0, E);
 
     if (!String1.checkNullPointerForFoldAccess(Info, E, AK_Read) ||
         !String2.checkNullPointerForFoldAccess(Info, E, AK_Read) ||

clang/test/CodeGen/gh64876.cpp

+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+
+void f(const char* C, const wchar_t *WC) {
+  int x1 = __builtin_strncmp(C, "b", 0xffffffffffffffff);
+// CHECK: {{.*}}= call i32 @strncmp{{.*}}i64 noundef -1
+  int x2 = __builtin_memcmp(C, "b", 0xffffffffffffffff);
+// CHECK: {{.*}}= call i32 @memcmp{{.*}}i64 noundef -1
+  int x3 = __builtin_bcmp(C, "b", 0xffffffffffffffff);
+// CHECK: {{.*}}= call i32 @bcmp{{.*}}i64 noundef -1
+  int x4 = __builtin_wmemcmp(WC, L"b", 0xffffffffffffffff);
+// CHECK: {{.*}}= call i32 @wmemcmp{{.*}}i64 noundef -1
+  auto x5 = __builtin_memchr(C, (int)'a', 0xffffffffffffffff);
+// CHECK: {{.*}}= call ptr @memchr{{.*}}i64 noundef -1
+  auto x6 = __builtin_wmemchr(WC, (int)'a', 0xffffffffffffffff);
+// CHECK: {{.*}}= call ptr @wmemchr{{.*}}i64 noundef -1
+}

clang/test/SemaCXX/constexpr-string.cpp

constexpr bool test_address_of_incomplete_struct_type() { // expected-error {{never produces a constant}}
     struct Incomplete;
     extern Incomplete x, y;
     __builtin_memcpy(&x, &x, 4);
     // expected-note@-1 2{{cannot constant evaluate 'memcpy' between objects of incomplete type 'Incomplete'}}
     return true;
   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error {{constant}} expected-note {{in call}}
 }
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wconstant-conversion"
+namespace GH64876 {
+void f() {
+  __builtin_strncmp(0, 0, 0xffffffffffffffff);
+  __builtin_memcmp(0, 0, 0xffffffffffffffff);
+  __builtin_bcmp(0, 0, 0xffffffffffffffff);
+  __builtin_wmemcmp(0, 0, 0xffffffffffffffff);
+  __builtin_memchr((const void*)0, 1, 0xffffffffffffffff);
+  __builtin_wmemchr((const wchar_t*)0, 1, 0xffffffffffffffff);
+
+  __builtin_strncmp(0, 0, -511LL);
+  __builtin_memcmp(0, 0, -511LL);
+  __builtin_bcmp(0, 0, -511LL);
+  __builtin_wmemcmp(0, 0, -511LL);
+  __builtin_memchr((const void*)0, 1, -511LL);
+  __builtin_wmemchr((const wchar_t*)0, 1, -511LL);
+}
+}
+#pragma clang diagnostic pop