[libc-commits] [libc] [libc][printf] Fix %s precision handling (PR #233098)

[via libc-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libc

Author: Nathan Cornaud (cornaud)

<details>
<summary>Changes</summary>

This fixes an issue where the string converter for `%s` attempts to retrieve the string length of the argument by climbing to the null-terminator regardless of precision, even tho if a precision is specified the null-terminator isn't mandatory.

Now, the string length is calculated up to the null-terminator only if no precision is specified, otherwise the length calculation is capped up to the precision.

A test is added for %s conversion with a specified precision, checking if it correctly formats a non-null-terminated input string when the input contains at least the required number of characters.

However, it is important to note that this test does not guarantee that the out-of-bounds bug will not reoccur, as detecting it would require running the test under ASan, and the bug itself may not be reproducible deterministically.

Fixes #<!-- -->230110  .

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


2 Files Affected:

- (modified) libc/src/__support/printf_core/string_converter.h (+7-5) 
- (modified) libc/test/src/__support/printf_core/converter_test.cpp (+34) 


``````````diff
diff --git a/libc/src/__support/printf_core/string_converter.h b/libc/src/__support/printf_core/string_converter.h
index 4a72ee03d43ada..1f82ff79893694 100644
--- a/libc/src/__support/printf_core/string_converter.h
+++ b/libc/src/__support/printf_core/string_converter.h
@@ -40,11 +40,13 @@ LIBC_INLINE int write_char_string_as_chars(Writer<mode, char> *writer,
   }
 #endif // LIBC_COPT_PRINTF_NO_NULLPTR_CHECKS
 
-  string_len = internal::string_length(str_ptr);
-
-  if (to_conv.precision >= 0 &&
-      static_cast<size_t>(to_conv.precision) < string_len)
-    string_len = to_conv.precision;
+  // If a precision is specified, it doesn't have to be null-terminated.
+  if (to_conv.precision >= 0) {
+    while (string_len < static_cast<size_t>(to_conv.precision) &&
+           str_ptr[string_len] != '\0')
+      string_len++;
+  } else
+    string_len = internal::string_length(str_ptr);
 
   size_t padding_spaces = to_conv.min_width > static_cast<int>(string_len)
                               ? to_conv.min_width - string_len
diff --git a/libc/test/src/__support/printf_core/converter_test.cpp b/libc/test/src/__support/printf_core/converter_test.cpp
index facbda405acb7d..3f2a96e82a535c 100644
--- a/libc/test/src/__support/printf_core/converter_test.cpp
+++ b/libc/test/src/__support/printf_core/converter_test.cpp
@@ -279,6 +279,40 @@ TYPED_TEST(LlvmLibcPrintfConverterTest, StringConversionPrecisionLow,
   ASSERT_EQ(writer.get_chars_written(), size_t{2});
 }
 
+TYPED_TEST(LlvmLibcPrintfConverterTest, StringConversionNoNullTerminator,
+           TestCharTypes) {
+  // When a precision is specified for a %s conversion, the input string does
+  // not need to be null-terminated, provided it contains at least as many
+  // characters as the specified precision.
+
+  using CharT = ParamType;
+  CharT str[60];
+  Writer writer =
+      make_drop_overflow_writer(str, sizeof(str) / sizeof(CharT) - 1);
+  WriteBuffer<CharT> &wb = writer.get_write_buffer();
+
+  const CharT input[] = {ENCODED(CharT, 'j'), ENCODED(CharT, 'k'),
+                         ENCODED(CharT, 'l')};
+
+  FormatSection<CharT> no_null_terminator_conv;
+  no_null_terminator_conv.has_conv = true;
+  no_null_terminator_conv.raw_string = ENCODED(CharT, "%.3s");
+  no_null_terminator_conv.conv_name = ENCODED(CharT, 's');
+  no_null_terminator_conv.precision = 3;
+  if constexpr (LIBC_NAMESPACE::cpp::is_same_v<CharT, wchar_t>) {
+    no_null_terminator_conv.length_modifier =
+    LIBC_NAMESPACE::printf_core::LengthModifier::l;
+  }
+  no_null_terminator_conv.conv_val_ptr = const_cast<CharT *>(input);
+
+  LIBC_NAMESPACE::printf_core::convert(&writer, no_null_terminator_conv);
+
+  wb.buff[wb.buff_cur] = ENCODED(CharT, '\0');
+
+  ASSERT_STREQ(str, ENCODED(CharT, "jkl"));
+  ASSERT_EQ(writer.get_chars_written(), size_t{3});
+}
+
 TYPED_TEST(LlvmLibcPrintfConverterTest, StringConversionRightJustified,
            TestCharTypes) {
   using CharT = ParamType;

``````````

</details>


https://github.com/llvm/llvm-project/pull/233098