Skip to content

[libc] fix msan failure in mempcpy_test #75532

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

nickdesaulniers
Copy link
Member

Internal builds of the unittests with msan flagged mempcpy_test.

==6862==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x55e34d7d734a in length llvm-project/libc/src/__support/CPP/string_view.h:41:11
#1 0x55e34d7d734a in string_view llvm-project/libc/src/__support/CPP/string_view.h:71:24
#2 0x55e34d7d734a in __llvm_libc_9999_0_0_git::testing::Test::testStrEq(char const*, char const*, char const*, char const*, __llvm_libc_9999_0_0_git::testing::internal::Location) llvm-project/libc/test/UnitTest/LibcTest.cpp:284:13
#3 0x55e34d7d4e09 in LlvmLibcMempcpyTest_Simple::Run() llvm-project/libc/test/src/string/mempcpy_test.cpp:20:3
#4 0x55e34d7d6dff in __llvm_libc_9999_0_0_git::testing::Test::runTests(char const*) llvm-project/libc/test/UnitTest/LibcTest.cpp:133:8
#5 0x55e34d7d86e0 in main llvm-project/libc/test/UnitTest/LibcTestMain.cpp:21:10

SUMMARY: MemorySanitizer: use-of-uninitialized-value llvm-project/libc/src/__support/CPP/string_view.h:41:11 in length

What's going on here is that mempcpy_test.cpp's Simple test is using
ASSERT_STREQ with a partially initialized char array. ASSERT_STREQ calls
Test::testStrEq which constructs a cpp:string_view. That constructor calls the
private method cpp::string_view::length. When built with msan, the loop is
transformed into multi-byte access, which then fails upon access.

I took a look at libc++'s __constexpr_strlen which just calls
__builtin_strlen(). Replacing the implementation of cpp::string_view::length
with a call to __builtin_strlen() may still result in out of bounds access when
the test is built with msan.

It's not safe to use ASSERT_STREQ with a partially initialized array.
Initialize the whole array so that the test passes.

Internal builds of the unittests with msan flagged mempcpy_test.

    ==6862==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x55e34d7d734a in length llvm-project/libc/src/__support/CPP/string_view.h:41:11
    llvm#1 0x55e34d7d734a in string_view llvm-project/libc/src/__support/CPP/string_view.h:71:24
    llvm#2 0x55e34d7d734a in __llvm_libc_9999_0_0_git::testing::Test::testStrEq(char const*, char const*, char const*, char const*, __llvm_libc_9999_0_0_git::testing::internal::Location) llvm-project/libc/test/UnitTest/LibcTest.cpp:284:13
    llvm#3 0x55e34d7d4e09 in LlvmLibcMempcpyTest_Simple::Run() llvm-project/libc/test/src/string/mempcpy_test.cpp:20:3
    llvm#4 0x55e34d7d6dff in __llvm_libc_9999_0_0_git::testing::Test::runTests(char const*) llvm-project/libc/test/UnitTest/LibcTest.cpp:133:8
    llvm#5 0x55e34d7d86e0 in main llvm-project/libc/test/UnitTest/LibcTestMain.cpp:21:10

    SUMMARY: MemorySanitizer: use-of-uninitialized-value llvm-project/libc/src/__support/CPP/string_view.h:41:11 in length

What's going on here is that mempcpy_test.cpp's Simple test is using
ASSERT_STREQ with a partially initialized char array. ASSERT_STREQ calls
Test::testStrEq which constructs a cpp:string_view. That constructor calls the
private method cpp::string_view::length. When built with msan, the loop is
transformed into multi-byte access, which then fails upon access.

I took a look at libc++'s __constexpr_strlen which just calls
__builtin_strlen().  Replacing the implementation of cpp::string_view::length
with a call to __builtin_strlen() may still result in out of bounds access when
the test is built with msan.

It's not safe to use ASSERT_STREQ with a partially initialized array.
Initialize the whole array so that the test passes.
@llvmbot llvmbot added the libc label Dec 14, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 14, 2023

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

Internal builds of the unittests with msan flagged mempcpy_test.

==6862==WARNING: MemorySanitizer: use-of-uninitialized-value
#<!-- -->0 0x55e34d7d734a in length llvm-project/libc/src/__support/CPP/string_view.h:41:11
#<!-- -->1 0x55e34d7d734a in string_view llvm-project/libc/src/__support/CPP/string_view.h:71:24
#<!-- -->2 0x55e34d7d734a in __llvm_libc_9999_0_0_git::testing::Test::testStrEq(char const*, char const*, char const*, char const*, __llvm_libc_9999_0_0_git::testing::internal::Location) llvm-project/libc/test/UnitTest/LibcTest.cpp:284:13
#<!-- -->3 0x55e34d7d4e09 in LlvmLibcMempcpyTest_Simple::Run() llvm-project/libc/test/src/string/mempcpy_test.cpp:20:3
#<!-- -->4 0x55e34d7d6dff in __llvm_libc_9999_0_0_git::testing::Test::runTests(char const*) llvm-project/libc/test/UnitTest/LibcTest.cpp:133:8
#<!-- -->5 0x55e34d7d86e0 in main llvm-project/libc/test/UnitTest/LibcTestMain.cpp:21:10

SUMMARY: MemorySanitizer: use-of-uninitialized-value llvm-project/libc/src/__support/CPP/string_view.h:41:11 in length

What's going on here is that mempcpy_test.cpp's Simple test is using
ASSERT_STREQ with a partially initialized char array. ASSERT_STREQ calls
Test::testStrEq which constructs a cpp:string_view. That constructor calls the
private method cpp::string_view::length. When built with msan, the loop is
transformed into multi-byte access, which then fails upon access.

I took a look at libc++'s __constexpr_strlen which just calls
__builtin_strlen(). Replacing the implementation of cpp::string_view::length
with a call to __builtin_strlen() may still result in out of bounds access when
the test is built with msan.

It's not safe to use ASSERT_STREQ with a partially initialized array.
Initialize the whole array so that the test passes.


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

1 Files Affected:

  • (modified) libc/test/src/string/mempcpy_test.cpp (+1-1)
diff --git a/libc/test/src/string/mempcpy_test.cpp b/libc/test/src/string/mempcpy_test.cpp
index 43ad5a0f3e1b7e..877ee8104880eb 100644
--- a/libc/test/src/string/mempcpy_test.cpp
+++ b/libc/test/src/string/mempcpy_test.cpp
@@ -14,7 +14,7 @@
 // mempcpy behavior (returning the end of what was copied).
 TEST(LlvmLibcMempcpyTest, Simple) {
   const char *src = "12345";
-  char dest[10];
+  char dest[10] = {};
   void *result = LIBC_NAMESPACE::mempcpy(dest, src, 6);
   ASSERT_EQ(static_cast<char *>(result), dest + 6);
   ASSERT_STREQ(src, dest);

@nickdesaulniers nickdesaulniers merged commit 7c6b4be into llvm:main Dec 14, 2023
@nickdesaulniers nickdesaulniers deleted the msan_mempcpy branch December 14, 2023 21:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants