[libc] add integer writing to printf

[michaelrj]

Summary

This patch adds %n to printf, as well as a compiler flag to disable it.
This is due to it having serious security issues when misused.

[tschuett]

I know that you are tied to the standards. Maybe expose llvmllibc extensions of safe or hardened functions, e.g., save_printf.

[michaelrj]

While the idea of having a seperate "safe" version of printf is not inherently bad, it's already been tried to little success. In the standard right now is the definitions for the _s versions of printf functions. These have bounds checking and disable %n, but are also not widely used. Convincing developers to switch is hard, so my current plan is to use the compile-time switch I've added to disable %n by default in the existing printf functions. This provides the security enhancements by default for all of the people who don't care/didn't know that %n existed, while still allowing people to turn it back on if they are really sure they know what they're doing.

[dxf]

In D127517#3574383, @tschuett wrote:

I know that you are tied to the standards. Maybe expose llvmllibc extensions of safe or hardened functions, e.g., save_printf.

Some libc implementations support compiling sources with FORTIFY_SOURCE, which (with compiler support) can call _chk() versions of functions like printf().

How best to support FORTIFY_SOURCE is an open question but it is something that is being thought about. Is this an area you are interested in helping with?

[tschuett]

Clang can also do things with _FORTIFY_SOURCE:
https://reviews.llvm.org/D58797

[dxf]

In D127517#3574573, @tschuett wrote:

Clang can also do things with _FORTIFY_SOURCE:
https://reviews.llvm.org/D58797

And that's one potential approach, to do more things with FORTIFY_SOURCE in Clang (and not require libc to carry special FORTIFY bits).

[tschuett]

Do you really need compiler support?

#ifdef FORTIFY_SOURCE
#include "checked_printf.h"
#else
#include "fast_printf.h"
#endif

[dxf]

FORTIFY_SOURCE support has typically been provided through a combination of compiler and libc support (e.g. gcc and glibc, Clang and bionic). Multiple applications can link against the same libc, and some will want the FORTIFY_SOURCE versions and some will not, but all user code still just wants to call "mempcpy()" or whatever. When FORTIFY_SOURCE is enabled, the compiler can know to pick the _chk versions of the functions (and do some other work as well).

[tschuett]

Maybe I'm stupid, but I would expect both headers to expose a printf.

int printf(const char * restrict format, ...) {
  do some checks here
}

[dxf]

You could certainly do something like that.

FORTIFY_SOURCE is a long-established way of inserting a relatively low-cost runtime check to a number of libc functions. The compiler can also do some static checks and it can also decide things like "should this call to mempcpy() invoke the regular mempcpy(), or the one with extra checks?" and not simply call the one with extra checks every time.

For many projects, a "hardened" libc is nice but not sufficient, and they use tools like ASan instead (which can help catch memory problems across all the code and not just in the libc functions).

I think if we want to have "safe" versions of these functions we should do it via the FORTIFY_SOURCE mechanism since both glibc and bionic support it. LLVM's libc could carry code (like glibc and bionic) to support FORTIFY, if someone wants to contribute those patches. There might be other ways to achieve the same functionality. I'd like to continue this discussion, but that's probably best done on discourse.

[michaelrj]

rebase onto new changes, specifically converters getting return values.

[lntue]

libc/src/stdio/printf_core/write_int_converter.h

25

How is this return value defined? Aren't errors normally -1? And do we have tests for this?

[michaelrj]

add test for the nullptr check

libc/src/stdio/printf_core/write_int_converter.h

25

this check is not in any of the specifications, and is a convenience feature that I added. The return value is negative, since the return value for printf is negative on errors. As for why it's -3, that's because -1 and -2 are used for file writing errors, and this allows me as the developer to tell what the error was.

I have now added a test to check that it returns an error value when passed a null pointer.

[lntue]

libc/src/stdio/printf_core/write_int_converter.h

25

Sound like we'll need some commonly defined macros for these values as the number of different errors grows.

[michaelrj]

add comment explaining the negative return value.

libc/src/stdio/printf_core/write_int_converter.h

25

I don't expect the number of errors to grow much, but I will make a followup patch adding the error values to core_structs.h.

libc/src/stdio/printf_core/CMakeLists.txt

HDRS
     converter_atlas.h
     converter_utils.h
     string_converter.h
     char_converter.h
     int_converter.h
     hex_converter.h
     ptr_converter.h
     oct_converter.h
+    write_int_converter.h
   DEPENDS
     .writer
     .core_structs
 )
 
 
 add_object_library(
   printf_main

libc/src/stdio/printf_core/converter.cpp

int convert(Writer *writer, const FormatSection &to_conv) {
   case 'E':
     // return convert_float_dec_exp(writer, to_conv);
   case 'a':
   case 'A':
     // return convert_float_hex_exp(writer, to_conv);
   case 'g':
   case 'G':
     // return convert_float_mixed(writer, to_conv);
-  // TODO(michaelrj): add a flag to disable writing an int here
+#ifndef LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
   case 'n':
-    // return convert_write_int(writer, to_conv);
+    return convert_write_int(writer, to_conv);
+#endif // LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
   case 'p':
     return convert_pointer(writer, to_conv);
   default:
     return writer->write(to_conv.raw_string, to_conv.raw_len);
   }
   return -1;
 }
 
 } // namespace printf_core
 } // namespace __llvm_libc

libc/src/stdio/printf_core/converter_atlas.h

 #include "src/stdio/printf_core/hex_converter.h"
 
 // TODO(michaelrj): add a flag to disable float point values here
 // defines convert_float_decimal
 // defines convert_float_dec_exp
 // defines convert_float_hex_exp
 // defines convert_float_mixed
 
-// TODO(michaelrj): add a flag to disable writing an int here
-// defines convert_write_int
+#ifndef LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
+#include "src/stdio/printf_core/write_int_converter.h"
+#endif // LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
 
 // defines convert_pointer
 #include "src/stdio/printf_core/ptr_converter.h"
 
 #endif // LLVM_LIBC_SRC_STDIO_PRINTF_CORE_CONVERTER_ATLAS_H

libc/src/stdio/printf_core/parser.cpp

#endif // LLVM_LIBC_PRINTF_DISABLE_INDEX_MODE
     case ('G'):
       if (lm != LengthModifier::L)
         section.conv_val_raw =
             bit_cast<uint64_t>(GET_ARG_VAL_SIMPLEST(double, conv_index));
       else
         section.conv_val_raw = bit_cast<fputil::FPBits<long double>::UIntType>(
             GET_ARG_VAL_SIMPLEST(long double, conv_index));
       break;
+#ifndef LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
     case ('n'):
+#endif // LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
     case ('p'):
     case ('s'):
       section.conv_val_ptr = GET_ARG_VAL_SIMPLEST(void *, conv_index);
       break;
     default:
       // if the conversion is undefined, change this to a raw section.
       section.has_conv = false;
       break;
if (str[local_pos] == '%') {
       case ('A'):
       case ('g'):
       case ('G'):
         if (lm != LengthModifier::L)
           conv_size = TYPE_DESC<double>;
         else
           conv_size = TYPE_DESC<long double>;
         break;
+#ifndef LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
       case ('n'):
+#endif // LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
       case ('p'):
       case ('s'):
         conv_size = TYPE_DESC<void *>;
         break;
       default:
         conv_size = TYPE_DESC<int>;
         break;
       }

libc/src/stdio/printf_core/write_int_converter.h

+//===-- Write integer Converter for printf ----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_WRITE_INT_CONVERTER_H
+#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_WRITE_INT_CONVERTER_H
+
+#include "src/__support/CPP/Limits.h"
+#include "src/stdio/printf_core/core_structs.h"
+#include "src/stdio/printf_core/writer.h"
+
+#include <inttypes.h>
+#include <stddef.h>
+
+namespace __llvm_libc {
+namespace printf_core {
+
+int inline convert_write_int(Writer *writer, const FormatSection &to_conv) {
+
+  // This is an additional check added by LLVM-libc. The reason it returns -3 is
+  // because printf uses negative return values for errors, and -1 and -2 are
+  // already in use by the file_writer class for file errors.
+  if (to_conv.conv_val_ptr == nullptr)
+    return -3;
+
+  int written = writer->get_chars_written();
+
+  switch (to_conv.length_modifier) {
+  case LengthModifier::none:
+    *reinterpret_cast<int *>(to_conv.conv_val_ptr) = written;
+    break;
+  case LengthModifier::l:
+    *reinterpret_cast<long *>(to_conv.conv_val_ptr) = written;
+    break;
+  case LengthModifier::ll:
+  case LengthModifier::L:
+    *reinterpret_cast<long long *>(to_conv.conv_val_ptr) = written;
+    break;
+  case LengthModifier::h:
+    *reinterpret_cast<short *>(to_conv.conv_val_ptr) = written;
+    break;
+  case LengthModifier::hh:
+    *reinterpret_cast<signed char *>(to_conv.conv_val_ptr) = written;
+    break;
+  case LengthModifier::z:
+    *reinterpret_cast<size_t *>(to_conv.conv_val_ptr) = written;
+    break;
+  case LengthModifier::t:
+    *reinterpret_cast<ptrdiff_t *>(to_conv.conv_val_ptr) = written;
+    break;
+  case LengthModifier::j:
+    *reinterpret_cast<uintmax_t *>(to_conv.conv_val_ptr) = written;
+    break;
+  }
+  return 0;
+}
+
+} // namespace printf_core
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_STDIO_PRINTF_CORE_WRITE_INT_CONVERTER_H

libc/test/src/stdio/sprintf_test.cpp

TEST(LlvmLibcSPrintfTest, IntConv) {
   ASSERT_STREQ(buff, "123");
 
   written = __llvm_libc::sprintf(buff, "%i", -456);
   EXPECT_EQ(written, 4);
   ASSERT_STREQ(buff, "-456");
 
   // Length Modifier Tests.
 
-  written = __llvm_libc::sprintf(buff, "%hhu", 257); // 0x10001
+  written = __llvm_libc::sprintf(buff, "%hhu", 257); // 0x101
   EXPECT_EQ(written, 1);
   ASSERT_STREQ(buff, "1");
 
   written = __llvm_libc::sprintf(buff, "%llu", 18446744073709551615ull);
   EXPECT_EQ(written, 20);
   ASSERT_STREQ(buff, "18446744073709551615"); // ull max
 
   written = __llvm_libc::sprintf(buff, "%tu", ~ptrdiff_t(0));
TEST(LlvmLibcSPrintfTest, OctConv) {
   ASSERT_STREQ(buff, "0075 0025");
 
   written = __llvm_libc::sprintf(buff, "%04hho %#.5llo %-6.3zo", 256 + 077,
                                  01000000000000ll, size_t(2));
   EXPECT_EQ(written, 26);
   ASSERT_STREQ(buff, "0077 01000000000000 002   ");
 }
 
+#ifndef LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
+TEST(LlvmLibcSPrintfTest, WriteIntConv) {
+  char buff[64];
+  int written;
+  int test_val = -1;
+
+  test_val = -1;
+  written = __llvm_libc::sprintf(buff, "12345%n67890", &test_val);
+  EXPECT_EQ(written, 10);
+  EXPECT_EQ(test_val, 5);
+  ASSERT_STREQ(buff, "1234567890");
+
+  test_val = -1;
+  written = __llvm_libc::sprintf(buff, "%n", &test_val);
+  EXPECT_EQ(written, 0);
+  EXPECT_EQ(test_val, 0);
+  ASSERT_STREQ(buff, "");
+
+  test_val = 0x100;
+  written = __llvm_libc::sprintf(buff, "ABC%hhnDEF", &test_val);
+  EXPECT_EQ(written, 6);
+  EXPECT_EQ(test_val, 0x103);
+  ASSERT_STREQ(buff, "ABCDEF");
+
+  test_val = -1;
+  written = __llvm_libc::sprintf(buff, "%s%n", "87654321", &test_val);
+  EXPECT_EQ(written, 8);
+  EXPECT_EQ(test_val, 8);
+  ASSERT_STREQ(buff, "87654321");
+
+  written = __llvm_libc::sprintf(buff, "abc123%n", nullptr);
+  EXPECT_LT(written, 0);
+}
+#endif // LLVM_LIBC_PRINTF_DISABLE_WRITE_INT
+
 #ifndef LLVM_LIBC_PRINTF_DISABLE_INDEX_MODE
 TEST(LlvmLibcSPrintfTest, IndexModeParsing) {
   char buff[64];
   int written;
 
   written = __llvm_libc::sprintf(buff, "%1$s", "abcDEF123");
   EXPECT_EQ(written, 9);
   ASSERT_STREQ(buff, "abcDEF123");