[libc] Expand usage of libc null checks. #116262

AlyElashram · 2024-11-14T17:51:00Z

AlyElashram · 2024-12-18T20:49:30Z

Hey @nickdesaulniers , I'm done with the changes however I'm currently creating unit tests to check that the functions segfault correctly. I'm failing to catch the segfault gracefully. Here's my current implementation which segfaults and coredumps for the memchr.cpp function.

#include "hdr/signal_macros.h"

TEST(LlvmLibcMemChrTest, CrashOnNullPtr) {
    LIBC_NAMESPACE::memchr(nullptr, 1, 1);
}

I tried the ASSERT_DEATH(LIBC_NAMESPACE::memchr(nullptr,1,1), WITH_SIGNAL(SIGSEGV)); macro but it causes linking errors.

nickdesaulniers · 2024-12-18T21:59:02Z

but it causes linking errors.

What's the error you observe? The precommit bot is pointing out:

/home/runner/work/llvm-project/llvm-project/libc/src/stdio/fscanf.cpp:25:3: error: variable has incomplete type 'volatile ::FILE' (aka 'volatile FILE')
   25 |   LIBC_CRASH_ON_NULLPTR(stream);
      |   ^
/home/runner/work/llvm-project/llvm-project/libc/src/__support/macros/null_check.h:23:38: note: expanded from macro 'LIBC_CRASH_ON_NULLPTR'
   23 |       [[maybe_unused]] volatile auto crash = *crashing;                        \
      |                                      ^
/home/runner/work/llvm-project/llvm-project/libc/include/llvm-libc-types/FILE.h:12:16: note: forward declaration of 'FILE'
   12 | typedef struct FILE FILE;
      |                ^

which isn't a linkage failure.

AlyElashram · 2024-12-18T22:44:26Z

What's the error you observe? The precommit bot is pointing out:

I reverted the changes in the functions that were failing in the prebuilt commit , My only failures now are in the tests.

Here is the test I'm trying out , the test is added inside memchr_test.cpp:

#include "hdr/signal_macros.h"
#include "test/UnitTest/Test.h"

TEST(LlvmLibcMemChrTest, CrashOnNullPtr) {
  ASSERT_DEATH([](){ LIBC_NAMESPACE::memchr(nullptr, 1, 1); } , WITH_SIGNAL(SIGSEGV));
}

Here is the result of the test :

[1908/2035] Linking CXX executable libc/test/src/string/libc.test.src.string.memchr_test.__hermetic__.__build__
FAILED: libc/test/src/string/libc.test.src.string.memchr_test.__hermetic__.__build__ 
: && /usr/bin/clang++ -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -g -nolibc -nostartfiles -nostdlib++ -static libc/startup/linux/crt1.o libc/test/src/string/CMakeFiles/libc.test.src.string.memchr_test.__hermetic__.__build__.dir/memchr_test.cpp.o -o libc/test/src/string/libc.test.src.string.memchr_test.__hermetic__.__build__  libc/test/UnitTest/libLibcTest.hermetic.a  libc/test/UnitTest/libLibcHermeticTestSupport.hermetic.a  libc/test/src/string/liblibc.test.src.string.memchr_test.__hermetic__.libc.a && :
/usr/bin/ld: libc/test/src/string/CMakeFiles/libc.test.src.string.memchr_test.__hermetic__.__build__.dir/memchr_test.cpp.o: in function `LlvmLibcMemChrTest_CrashOnNullPtr::Run()':
/home/aly/llvm-project/libc/test/src/string/memchr_test.cpp:127:(.text+0xcb2): undefined reference to `__llvm_libc_20_0_0_git::testing::Test::testProcessKilled(__llvm_libc_20_0_0_git::testutils::FunctionCaller*, int, char const*, char const*, __llvm_libc_20_0_0_git::testing::internal::Location)'
/usr/bin/ld: libc/test/src/string/libc.test.src.string.memchr_test.__hermetic__.__build__: hidden symbol `_ZN22__llvm_libc_20_0_0_git7testing4Test17testProcessKilledEPNS_9testutils14FunctionCallerEiPKcS6_NS0_8internal8LocationE' isn't defined
/usr/bin/ld: final link failed: bad value
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

What I understand from the error is that it's not seeing the ASSERT_DEATH macro , but i'm not sure why since the test.h file is included.
@nickdesaulniers

nickdesaulniers · 2024-12-19T18:31:03Z

@michaelrj-google pointed out to me that any tests that you add usage of EXPECT_DEATH or ASSERT_DEATH, you'll need to add UNIT_TEST_ONLY to their cmake add_libc_test invocation.

Example:

llvm-project/libc/test/src/stdlib/CMakeLists.txt

Line 409 in 2886576

UNIT_TEST_ONLY

Sounds like that should disable the hermetic test for that function, which is probably fine.

libc/cmake/modules/LLVMLibCTestRules.cmake

libc/test/src/string/CMakeLists.txt

github-actions · 2024-12-22T22:18:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

libc/src/string/memchr.cpp

libc/test/src/string/memcpy_test.cpp

libc/test/src/string/memchr_test.cpp

libc/test/src/string/strcat_test.cpp

libc/test/src/string/strspn_test.cpp

libc/test/src/string/strcoll_test.cpp

libc/test/src/string/mempcpy_test.cpp

libc/test/src/string/memrchr_test.cpp

nickdesaulniers · 2025-01-23T20:48:09Z

#123401 has been merged; please rebase on top of that. Then you can drop the extra temporary variables.

AlyElashram · 2025-01-23T22:21:22Z

#123401 has been merged; please rebase on top of that. Then you can drop the extra temporary variables.

Will patch it first thing tomorrow, I'll also remove checking for the signal and just leave the expect_death macro

libc/src/string/memcmp.cpp

libc/src/string/memccpy.cpp

libc/src/string/memchr.cpp

libc/src/string/memory_utils/inline_strstr.h

libc/src/string/strsep.cpp

libc/test/src/string/CMakeLists.txt

libc/test/src/string/memchr_test.cpp

libc/src/string/memchr.cpp

libc/src/string/memory_utils/inline_strstr.h

libc/src/string/strsep.cpp

libc/test/src/string/CMakeLists.txt

libc/src/string/memccpy.cpp

lntue · 2025-04-17T07:54:14Z

@AlyElashram do you mind syncing the PR to resolve conflicts?

AlyElashram · 2025-04-19T16:05:35Z

@lntue can you please check the tests out when you have the time 👀

… to SIGTRAP

libc/test/src/string/memchr_test.cpp

libc/test/src/string/memcmp_test.cpp

lntue · 2025-05-31T17:35:55Z

libc/src/string/memrchr.cpp

 #include <stddef.h>

 namespace LIBC_NAMESPACE_DECL {

 LLVM_LIBC_FUNCTION(void *, memrchr, (const void *src, int c, size_t n)) {
+  LIBC_CRASH_ON_NULLPTR(src);


Do we need to check if n != 0 before checking nullptr?

These functions are not mentioned in this doc here , but they are different variants of the same function.

memrchr for example , is a variant of memchr and memchr is mentioned in the doc.

I'm not sure if we should include the variants or follow the document strictly @lntue .

These functions are probably GNU or BSD extensions and are not specified in the C standards, so technically we can define any behavior that we think it makes sense. Nonetheless, the implementation below is not UB and well-behaved when n = 0, in addition to being consistent with memchr.

libc/src/string/stpncpy.cpp

libc/src/string/memset.cpp

libc/test/src/string/CMakeLists.txt

lntue · 2025-06-02T20:04:48Z

libc/test/src/string/memcpy_test.cpp

+  ASSERT_DEATH([]() { LIBC_NAMESPACE::memcpy(nullptr, nullptr, 1); },
+               WITH_SIGNAL(-1));
+}
+#endif // defined(LIBC_TARGET_OS_IS_LINUX)


Nit: You might want to do mass update the comments.

AlyElashram · 2025-06-04T13:32:51Z

libc/src/string/mempcpy.cpp

@@ -18,6 +19,10 @@ namespace LIBC_NAMESPACE_DECL {
 LLVM_LIBC_FUNCTION(void *, mempcpy,
                   (void *__restrict dst, const void *__restrict src,
                    size_t count)) {
+  if (count) {


I added a count check here , for consistency with memcpy and also since memcpy and mempcpy are well behaved at n = 0
@lntue

AlyElashram · 2025-06-06T13:28:53Z

@lntue the unit tests are generating failures here that I don't understand. I'm not sure if this should've been merged. They generated errors that are also affecting all other PRs.

I traced back the errors and they only occur on this PR , and not #142085

Failure in question :

[6320/6668] Running unit test libc.test.src.string.memcmp_opt_host_test.__unit__
FAILED: projects/libc/test/src/string/CMakeFiles/libc.test.src.string.memcmp_opt_host_test.__unit__ /home/gha/actions-runner/_work/llvm-project/llvm-project/build/projects/libc/test/src/string/CMakeFiles/libc.test.src.string.memcmp_opt_host_test.__unit__

lntue · 2025-06-06T14:31:42Z

@lntue the unit tests are generating failures here that I don't understand. I'm not sure if this should've been merged. They generated errors that are also affecting all other PRs.

I traced back the errors and they only occur on this PR , and not #142085

Failure in question :
[6320/6668] Running unit test libc.test.src.string.memcmp_opt_host_test.__unit__
FAILED: projects/libc/test/src/string/CMakeFiles/libc.test.src.string.memcmp_opt_host_test.__unit__ /home/gha/actions-runner/_work/llvm-project/llvm-project/build/projects/libc/test/src/string/CMakeFiles/libc.test.src.string.memcmp_opt_host_test.__unit__ 

bzero and bcmp tests also failed in the precommit CI for similar errors (Illegal instructions on SizeSweep tests). Those 2 are not related to your change at all. Also post commit CIs are happy with the change, and right now I cannot reproduce these failures locally. It's probably for other reasons than this change.

AlyElashram changed the title ~~Expand usage of libc null checks~~ [libc] Expand usage of libc null checks. Dec 13, 2024

AlyElashram force-pushed the Expand-usage-of-libc-null-checks branch from e895dce to f7da133 Compare December 13, 2024 16:53

AlyElashram marked this pull request as ready for review December 13, 2024 16:53

AlyElashram marked this pull request as draft December 13, 2024 20:34

nickdesaulniers self-requested a review December 18, 2024 21:55

AlyElashram commented Dec 22, 2024

View reviewed changes

libc/cmake/modules/LLVMLibCTestRules.cmake Outdated Show resolved Hide resolved

libc/test/src/string/CMakeLists.txt Outdated Show resolved Hide resolved

AlyElashram marked this pull request as ready for review December 27, 2024 13:39

nickdesaulniers reviewed Jan 7, 2025

View reviewed changes

libc/src/string/memchr.cpp Outdated Show resolved Hide resolved