Skip to content

Reland "Fix wcpncpy() return value; add test." #146753

New issue

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

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

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

enh-google
Copy link
Contributor

Reverts #146752, which was a revert of my accidental push, so we can actually review and presubmit this time.

@llvmbot llvmbot added the libc label Jul 2, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 2, 2025

@llvm/pr-subscribers-libc

Author: None (enh-google)

Changes

Reverts llvm/llvm-project#146752, which was a revert of my accidental push, so we can actually review and presubmit this time.


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

2 Files Affected:

  • (modified) libc/src/wchar/wcpncpy.cpp (+3-2)
  • (modified) libc/test/src/wchar/wcpncpy_test.cpp (+9)
diff --git a/libc/src/wchar/wcpncpy.cpp b/libc/src/wchar/wcpncpy.cpp
index 9f451b73f07cb..0e729db7abf60 100644
--- a/libc/src/wchar/wcpncpy.cpp
+++ b/libc/src/wchar/wcpncpy.cpp
@@ -28,8 +28,9 @@ LLVM_LIBC_FUNCTION(wchar_t *, wcpncpy,
   for (i = 0; i < n && s2[i] != '\0'; ++i)
     s1[i] = s2[i];
   // When n>strlen(src), n-strlen(src) \0 are appended.
-  for (; i < n; ++i)
-    s1[i] = L'\0';
+  for (size_t j = i; j < n; ++j)
+    s1[j] = L'\0';
+  // ...but our result points to the first \0 (if any).
   return s1 + i;
 }
 
diff --git a/libc/test/src/wchar/wcpncpy_test.cpp b/libc/test/src/wchar/wcpncpy_test.cpp
index 98738e230e32d..5ce3e8ce7cd93 100644
--- a/libc/test/src/wchar/wcpncpy_test.cpp
+++ b/libc/test/src/wchar/wcpncpy_test.cpp
@@ -75,6 +75,15 @@ TEST(LlvmLibcWCPNCpyTest, CopyTwoWithNull) {
   ASSERT_EQ(dest + 2, res);
 }
 
+TEST(LlvmLibcWCPNCpyTest, CopyAndFill) {
+  wchar_t dest[] = {L'a', L'b', L'c'};
+  wchar_t *res = LIBC_NAMESPACE::wcpncpy(dest, L"x", 3);
+  ASSERT_TRUE(dest[0] == L'x');
+  ASSERT_TRUE(dest[1] == L'\0');
+  ASSERT_TRUE(dest[2] == L'\0');
+  ASSERT_EQ(dest + 1, res);
+}
+
 #if defined(LIBC_ADD_NULL_CHECKS) && !defined(LIBC_HAS_SANITIZER)
 TEST(LlvmLibcWCPNCpyTest, NullptrCrash) {
   // Passing in a nullptr should crash the program.

@enh-google
Copy link
Contributor Author

i've also sent a fix to the man page upstream, since the man page has the same bug: https://lore.kernel.org/linux-man/CAJgzZoq0AvK8EDicLk7ZMVbWS8MmoqW0Nv4U9HCFUXnNw+yUGw@mail.gmail.com/T/#u

https://pubs.opengroup.org/onlinepubs/9799919799/functions/wcpncpy.html (and the fact that this failed an existing bionic unit test -- that also passes when run against glibc -- when i tried to switch Android over to the llvm-libc wcpncpy :-) ) is my proof that the new behavior is the expected behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other tests need to be changed to match the new behavior of the function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants