[libc++] Enable -Wformat-nonliteral when building libc++

[ldionne]

Summary

Using user-provided data as a format string is a well known source of
security vulnerabilities. For this reason, it is a good idea to compile
our code with -Wformat-nonliteral, which basically warns if a non-constant
string is used as a format specifier. This is the compiler’s best signal
that a format string call may be insecure.

I audited the code after adding the warning and made sure that the few
places where we used a non-literal string as a format string were not
potential security issues. I disabled the warning locally for those
instances. The idea is that after we add the warning to the build, any
new use of a non-literal string in a format string will trigger a
diagnostic, and we can either get rid of it or disable the warning
locally, which is a way of acknowledging that it has been audited.

I also looked into enabling it in the test suite, which would perhaps
allow finding additional instances of it in our headers, however that
is not possible at the moment because Clang doesn't support putting
attribute((format(...))) on variadic templates, which would
be needed.

rdar://84571685

[• Quuxplusone]

because Clang doesn't support putting __attribute__((format(...))) on variadic templates

D112579 is related.

libcxx/include/locale

1617

Why on earth is this not just s/__fmt/"%p"/?

[ldionne]

Address review comment

[uabelho]

Hi,

I see a whole bunch of warnings like this with this patch:

/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:116:37: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vsnprintf(__s, __n, __format, __va);
                                    ^~~~~~~~
/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:126:32: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vasprintf(__s, __format, __va);
                               ^~~~~~~~
/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:136:30: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vsscanf(__s, __format, __va);
                             ^~~~~~~~
3 warnings generated.

Both with clang 8 and gcc 9.3

[uabelho]

In D112927#3123920, @uabelho wrote:

Hi,

I see a whole bunch of warnings like this with this patch:

/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:116:37: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vsnprintf(__s, __n, __format, __va);
                                    ^~~~~~~~
/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:126:32: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vasprintf(__s, __format, __va);
                               ^~~~~~~~
/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:136:30: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vsscanf(__s, __format, __va);
                             ^~~~~~~~
3 warnings generated.

Both with clang 8 and gcc 9.3

I put up
https://reviews.llvm.org/D113876
to silence the warnings

[ldionne]

In D112927#3130882, @uabelho wrote:

In D112927#3123920, @uabelho wrote:

Hi,

I see a whole bunch of warnings like this with this patch:

/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:116:37: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vsnprintf(__s, __n, __format, __va);
                                    ^~~~~~~~
/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:126:32: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vasprintf(__s, __format, __va);
                               ^~~~~~~~
/repo/uabelho/llvm-dev2/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_i386/include/c++/v1/__bsd_locale_fallbacks.h:136:30: warning: format string is not a string literal [-Wformat-nonliteral]
    int __res = vsscanf(__s, __format, __va);
                             ^~~~~~~~
3 warnings generated.

Both with clang 8 and gcc 9.3

I put up
https://reviews.llvm.org/D113876
to silence the warnings

Yup, I'm seeing them too on the build bots. I missed them cause I was only building on Darwin, and that header isn't used on Darwin. Also since those are just warnings they are not failing the build, so CI passed. Looking at your patch now.

libcxx/CMakeLists.txt

if (MSVC)
     # -W4 is the cl.exe/clang-cl equivalent of -Wall. (In cl.exe and clang-cl,
     # -Wall is equivalent to -Weverything in GCC style compiler drivers.)
     target_add_compile_flags_if_supported(${target} PRIVATE -W4)
   else()
     target_add_compile_flags_if_supported(${target} PRIVATE -Wall)
   endif()
   target_add_compile_flags_if_supported(${target} PRIVATE -Wextra -W -Wwrite-strings
                                                           -Wno-unused-parameter -Wno-long-long
-                                                          -Werror=return-type -Wextra-semi -Wundef)
+                                                          -Werror=return-type -Wextra-semi -Wundef
+                                                          -Wformat-nonliteral)
   if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
     target_add_compile_flags_if_supported(${target} PRIVATE
       -Wno-user-defined-literals
       -Wno-covered-switch-default
       -Wno-suggest-override
     )
     if (LIBCXX_TARGETING_CLANG_CL)
       target_add_compile_flags_if_supported(${target} PRIVATE

libcxx/include/locale

num_put<_CharT, _OutputIterator>::__do_put_integral(iter_type __s, ios_base& __iob,
     // Worst case is octal, with showbase enabled. Note that octal is always
     // printed as an unsigned value.
     using _Unsigned = typename make_unsigned<_Integral>::type;
     _LIBCPP_CONSTEXPR const unsigned __nbuf
         = (numeric_limits<_Unsigned>::digits / 3)        // 1 char per 3 bits
         + ((numeric_limits<_Unsigned>::digits % 3) != 0) // round up
         + 2; // base prefix + terminating null character
     char __nar[__nbuf];
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wformat-nonliteral"
     int __nc = __libcpp_snprintf_l(__nar, sizeof(__nar), _LIBCPP_GET_C_LOCALE, __fmt, __v);
+#pragma clang diagnostic pop
     char* __ne = __nar + __nc;
     char* __np = this->__identify_padding(__nar, __ne, __iob);
     // Stage 2 - Widen __nar while adding thousands separators
     char_type __o[2*(__nbuf-1) - 1];
     char_type* __op;  // pad here
     char_type* __oe;  // end of output
     this->__widen_and_group_int(__nar, __np, __ne, __o, __op, __oe, __iob.getloc());
     // [__o, __oe) contains thousands_sep'd wide number

 {
     // Stage 1 - Get number in narrow char
     char __fmt[8] = {'%', 0};
     bool __specify_precision = this->__format_float(__fmt+1, __len, __iob.flags());
     const unsigned __nbuf = 30;
     char __nar[__nbuf];
     char* __nb = __nar;
     int __nc;
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wformat-nonliteral"
     if (__specify_precision)
         __nc = __libcpp_snprintf_l(__nb, __nbuf, _LIBCPP_GET_C_LOCALE, __fmt,
                                    (int)__iob.precision(), __v);
     else
         __nc = __libcpp_snprintf_l(__nb, __nbuf, _LIBCPP_GET_C_LOCALE, __fmt, __v);
     unique_ptr<char, void(*)(void*)> __nbh(nullptr, free);
     if (__nc > static_cast<int>(__nbuf-1))
     {
         if (__specify_precision)
             __nc = __libcpp_asprintf_l(&__nb, _LIBCPP_GET_C_LOCALE, __fmt, (int)__iob.precision(), __v);
         else
             __nc = __libcpp_asprintf_l(&__nb, _LIBCPP_GET_C_LOCALE, __fmt, __v);
         if (__nc == -1)
             __throw_bad_alloc();
         __nbh.reset(__nb);
     }
+#pragma clang diagnostic pop
     char* __ne = __nb + __nc;
     char* __np = this->__identify_padding(__nb, __ne, __iob);
     // Stage 2 - Widen __nar while adding thousands separators
     char_type __o[2*(__nbuf-1) - 1];
     char_type* __ob = __o;
     unique_ptr<char_type, void(*)(void*)> __obh(0, free);
     if (__nb != __nar)
     {

 }
 
 template <class _CharT, class _OutputIterator>
 _OutputIterator
 num_put<_CharT, _OutputIterator>::do_put(iter_type __s, ios_base& __iob,
                                          char_type __fl, const void* __v) const
 {
     // Stage 1 - Get pointer in narrow char
-    char __fmt[6] = "%p";
     const unsigned __nbuf = 20;
     char __nar[__nbuf];
-    int __nc = __libcpp_snprintf_l(__nar, sizeof(__nar), _LIBCPP_GET_C_LOCALE, __fmt, __v);
+    int __nc = __libcpp_snprintf_l(__nar, sizeof(__nar), _LIBCPP_GET_C_LOCALE, "%p", __v);
     char* __ne = __nar + __nc;
     char* __np = this->__identify_padding(__nar, __ne, __iob);
     // Stage 2 - Widen __nar
     char_type __o[2*(__nbuf-1) - 1];
     char_type* __op;  // pad here
     char_type* __oe;  // end of output
     const ctype<char_type>& __ct = use_facet<ctype<char_type> >(__iob.getloc());
     __ct.widen(__nar, __ne, __o);