search

fmtlib / fmt

A modern formatting library
https://fmt.dev
Other
20.65k stars 2.48k forks source link

Format string checking incorrectly assumes repeated evaluation of string literal will produce the same pointer #4177

Closed zygoloid closed 3 weeks ago

zygoloid commented 3 weeks ago

In the fstring constructor from a type that is convertible to string view, the s argument is passed to two different functions, which both internally convert it to a string_view. This s argument is frequently created by the FMT_STRING macro which has a conversion to string_view that evaluates a string literal expression.

Each evaluation of a string literal expression can produce a different pointer. Therefore it is not correct to assume that the two conversions of s to a string_view produce related string_views, but the code does assume this. Historically, compilers have let you get away with this in compile-time evaluation, but clang recently gained a more strict implementation of this rule and now rejects.

I think the fix is probably to do the same thing in this constructor that is done a few lines below in the next constructor:

   template <typename S,
             FMT_ENABLE_IF(std::is_convertible<const S&, string_view>::value)>
   FMT_CONSTEVAL FMT_ALWAYS_INLINE fstring(const S& s) : str(s) {
     if (FMT_USE_CONSTEVAL)
+      FMT_CONSTEXPR auto sv = string_view(S());
-      detail::parse_format_string<char>(s, checker(s, arg_pack()));
+      detail::parse_format_string<char>(sv, checker(sv, arg_pack()));
 #ifdef FMT_ENFORCE_COMPILE_STRING
     static_assert(
         FMT_USE_CONSTEVAL && sizeof(s) != 0,
         "FMT_ENFORCE_COMPILE_STRING requires format strings to use FMT_STRING");
 #endif
   }
   template <typename S,
             FMT_ENABLE_IF(std::is_base_of<detail::compile_string, S>::value&&
                               std::is_same<typename S::char_type, char>::value)>
   FMT_ALWAYS_INLINE fstring(const S&) : str(S()) {
     FMT_CONSTEXPR auto sv = string_view(S());
     FMT_CONSTEXPR int ignore =
         (parse_format_string(sv, checker(sv, arg_pack())), 0);
     detail::ignore_unused(ignore);
   }
vitaut commented 3 weeks ago

Applied the patch (with a small tweak) in https://github.com/fmtlib/fmt/commit/cacc3108c5b74020dba7bf3c6d3a7e58cdc085b2. Thanks!

phprus commented 3 weeks ago

@vitaut Why

FMT_CONSTEXPR auto sv = string_view(S());

?

Instead of 

FMT_CONSTEXPR auto sv = string_view(s);

?

S() vs s variable.

vitaut commented 3 weeks ago

I don't think it matters much but since the object is already available, I don't see why we need to construct another one.

vitaut commented 3 weeks ago

Adding a regression test: https://github.com/fmtlib/fmt/commit/1c5883bef081a5cce75bdc4dab0ce922d3bb600f.

vitaut commented 3 weeks ago

I don't think it matters much but since the object is already available, I don't see why we need to construct another one.

Nevermind, I misinterpreted which is the current version of the code. You are right, we could use s there.

zeroomega commented 2 weeks ago

@zygoloid

I did some local test by rolling our fmtlib to ToT and building our project with ToT clang but still getting errors:

FAILED: host_x64/obj/third_party/fmtlib/src/src/fmtlib.format.cc.o                                                                                                                                                
../../../clang/clang_fmt/bin/clang++ -MD -MF host_x64/obj/third_party/fmtlib/src/src/fmtlib.format.cc.o.d -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -I../.. -Ihost_x64/gen -I.
./../third_party/fmtlib/src/include -fcolor-diagnostics -fcrash-diagnostics-dir=clang-crashreports -fcrash-diagnostics=all -gen-reproducer=error -ffp-contract=off --sysroot=../../prebuilt/third_party/sysroot/li
nux --target=x86_64-unknown-linux-gnu -ffile-compilation-dir=. -no-canonical-prefixes -fomit-frame-pointer -fdata-sections -ffunction-sections -O0 -Xclang -debug-info-kind=constructor -g3 -grecord-gcc-switches 
-gdwarf-5 -gz=zstd -Wall -Wextra -Wconversion -Wextra-semi -Wimplicit-fallthrough -Wnewline-eof -Wstrict-prototypes -Wwrite-strings -Wno-sign-conversion -Wno-unused-parameter -Wnonportable-system-include-path -
Wno-missing-field-initializers -Wno-extra-qualification -Wno-cast-function-type-strict -Wno-cast-function-type-mismatch -Wno-unknown-warning-option -Wno-missing-template-arg-list-after-template-kw -Wno-deprecat
ed-pragma -fvisibility=hidden -Werror -Wa,--fatal-warnings -Wno-error=deprecated-declarations --sysroot=../../prebuilt/third_party/sysroot/linux --target=x86_64-unknown-linux-gnu -fPIE -fvisibility-inlines-hidd
en -stdlib=libc++ -stdlib=libc++ -std=c++20 -Wno-deprecated-this-capture -fno-exceptions -fno-rtti -ftemplate-backtrace-limit=0 -stdlib=libc++ -c ../../third_party/fmtlib/src/src/format.cc -o host_x64/obj/third
_party/fmtlib/src/src/fmtlib.format.cc.o                                                                                                                                                                          
In file included from ../../third_party/fmtlib/src/src/format.cc:8:                                                                                                                                               
In file included from ../../third_party/fmtlib/src/include/fmt/format-inl.h:23:                                                                                                                                   
../../third_party/fmtlib/src/include/fmt/format.h:2677:4: error: extra ';' after member function definition [-Werror,-Wextra-semi]                                                                                
 2677 |   };                                                                                                                                                                                                      
      |    ^                                                                                                                                                                                                      
1 error generated.

and 

[49936/153879](31) CXX host_x64/obj/third_party/android/platform/system/tools/aidl/aidl_gen.aidl_to_cpp_common.cpp.o
FAILED: host_x64/obj/third_party/android/platform/system/tools/aidl/aidl_gen.aidl_to_cpp_common.cpp.o 
../../../clang/clang_fmt/bin/clang++ -MD -MF host_x64/obj/third_party/android/platform/system/tools/aidl/aidl_gen.aidl_to_cpp_common.cpp.o.d -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -I../../third_party/android/platform/system/tools/aidl -I../../src/lib/android/aidl/generated-files -I../../third_party/googletest/src/googletest/include -I../../third_party/googletest/src/googletest -I../.. -Ihost_x64/gen -I../../third_party/android/platform/system/libbase/include -I../../third_party/fmtlib/src/include -I../../third_party/android/platform/system/logging/liblog/include -I../../zircon/system/ulib/zx-panic-libc/include -I../../zircon/system/public -fcolor-diagnostics -fcrash-diagnostics-dir=clang-crashreports -fcrash-diagnostics=all -gen-reproducer=error -ffp-contract=off --sysroot=../../prebuilt/third_party/sysroot/linux --target=x86_64-unknown-linux-gnu -ffile-compilation-dir=. -no-canonical-prefixes -fomit-frame-pointer -fdata-sections -ffunction-sections -O0 -Xclang -debug-info-kind=constructor -g3 -grecord-gcc-switches -gdwarf-5 -gz=zstd -Wall -Wextra -Wconversion -Wextra-semi -Wimplicit-fallthrough -Wnewline-eof -Wstrict-prototypes -Wwrite-strings -Wno-sign-conversion -Wno-unused-parameter -Wnonportable-system-include-path -Wno-missing-field-initializers -Wno-extra-qualification -Wno-cast-function-type-strict -Wno-cast-function-type-mismatch -Wno-unknown-warning-option -Wno-missing-template-arg-list-after-template-kw -Wno-deprecated-pragma -fvisibility=hidden -Werror -Wa,--fatal-warnings -Wno-error=deprecated-declarations --sysroot=../../prebuilt/third_party/sysroot/linux --target=x86_64-unknown-linux-gnu -fPIE -fvisibility-inlines-hidden -stdlib=libc++ -stdlib=libc++ -std=c++20 -Wno-deprecated-this-capture -fno-exceptions -fno-rtti -ftemplate-backtrace-limit=0 -stdlib=libc++ -Wno-c++98-compat-extra-semi -Wno-c99-designator -Wno-deprecated-declarations -Wno-extra-semi -Wno-implicit-int-conversion -Wno-inconsistent-missing-override -Wno-newline-eof -Wno-range-loop-construct -Wno-reorder-init-list -Wno-shorten-64-to-32 -Wno-sign-compare -Wno-string-conversion -Wno-unused-but-set-variable -Wno-unused-result -Wno-unknown-warning-option -Wno-vla-cxx-extension -Wno-c++98-compat-extra-semi -Wno-c99-designator -Wno-deprecated-declarations -Wno-extra-semi -Wno-implicit-int-conversion -Wno-inconsistent-missing-override -Wno-newline-eof -Wno-range-loop-construct -Wno-reorder-init-list -Wno-shorten-64-to-32 -Wno-sign-compare -Wno-string-conversion -Wno-unused-but-set-variable -Wno-unused-result -c ../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp -o host_x64/obj/third_party/android/platform/system/tools/aidl/aidl_gen.aidl_to_cpp_common.cpp.o
In file included from ../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:16:
In file included from ../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.h:23:
In file included from ../../third_party/android/platform/system/tools/aidl/aidl_language.h:26:
In file included from ../../third_party/android/platform/system/libbase/include/android-base/result.h:104:
In file included from ../../third_party/android/platform/system/libbase/include/android-base/format.h:23:
In file included from ../../third_party/fmtlib/src/include/fmt/chrono.h:23:
In file included from ../../third_party/fmtlib/src/include/fmt/format.h:41:
../../third_party/fmtlib/src/include/fmt/base.h:2670:24: error: constexpr variable 'sv' must be initialized by a constant expression
 2670 |     FMT_CONSTEXPR auto sv = string_view(S());
      |                        ^    ~~~~~~~~~~~~~~~~
../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:374:24: note: in instantiation of function template specialization 'fmt::fstring<fmt::detail::named_arg<char, std::string>, fmt::detail::named_arg<char, std::string>, fmt::detail::named_arg<char, std::string>>::fstring<const char *, 0>' requested here
  374 |     out << fmt::format(tmpl, fmt::arg("name", name), fmt::arg("min_tag", min_tag),
      |                        ^
../../third_party/fmtlib/src/include/fmt/base.h:524:32: note: read of dereferenced null pointer is not allowed in a constant expression
  524 |       size_ = __builtin_strlen(detail::narrow(s));
      |                                ^
../../third_party/fmtlib/src/include/fmt/base.h:2670:29: note: in call to 'basic_string_view(nullptr)'
 2670 |     FMT_CONSTEXPR auto sv = string_view(S());
      |                             ^~~~~~~~~~~~~~~~
../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:374:24: error: call to consteval function 'fmt::fstring<fmt::detail::named_arg<char, std::string>, fmt::detail::named_arg<char, std::string>, fmt::detail::named_arg<char, std::string>>::fstring<const char *, 0>' is not a constant expression
  374 |     out << fmt::format(tmpl, fmt::arg("name", name), fmt::arg("min_tag", min_tag),
      |                        ^
../../third_party/fmtlib/src/include/fmt/base.h:524:32: note: read of dereferenced null pointer is not allowed in a constant expression
  524 |       size_ = __builtin_strlen(detail::narrow(s));
      |                                ^
../../third_party/fmtlib/src/include/fmt/base.h:2670:29: note: in call to 'basic_string_view(nullptr)'
 2670 |     FMT_CONSTEXPR auto sv = string_view(S());
      |                             ^~~~~~~~~~~~~~~~
../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:374:24: note: in call to 'fstring<const char *, 0>(tmpl)'
  374 |     out << fmt::format(tmpl, fmt::arg("name", name), fmt::arg("min_tag", min_tag),
      |                        ^~~~
In file included from ../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:16:
In file included from ../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.h:23:
In file included from ../../third_party/android/platform/system/tools/aidl/aidl_language.h:26:
In file included from ../../third_party/android/platform/system/libbase/include/android-base/result.h:104:
In file included from ../../third_party/android/platform/system/libbase/include/android-base/format.h:23:
In file included from ../../third_party/fmtlib/src/include/fmt/chrono.h:23:
In file included from ../../third_party/fmtlib/src/include/fmt/format.h:41:
../../third_party/fmtlib/src/include/fmt/base.h:2670:24: error: constexpr variable 'sv' must be initialized by a constant expression
 2670 |     FMT_CONSTEXPR auto sv = string_view(S());
      |                        ^    ~~~~~~~~~~~~~~~~
../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:735:24: note: in instantiation of function template specialization 'fmt::fstring<fmt::detail::named_arg<char, std::string>, fmt::detail::named_arg<char, std::string>>::fstring<const char *, 0>' requested here
  735 |     out << fmt::format(tmpl, fmt::arg("name", name), fmt::arg("typelist", typelist));
      |                        ^
../../third_party/fmtlib/src/include/fmt/base.h:524:32: note: read of dereferenced null pointer is not allowed in a constant expression
  524 |       size_ = __builtin_strlen(detail::narrow(s));
      |                                ^
../../third_party/fmtlib/src/include/fmt/base.h:2670:29: note: in call to 'basic_string_view(nullptr)'
 2670 |     FMT_CONSTEXPR auto sv = string_view(S());
      |                             ^~~~~~~~~~~~~~~~
../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:735:24: error: call to consteval function 'fmt::fstring<fmt::detail::named_arg<char, std::string>, fmt::detail::named_arg<char, std::string>>::fstring<const char *, 0>' is not a constant expression
  735 |     out << fmt::format(tmpl, fmt::arg("name", name), fmt::arg("typelist", typelist));
      |                        ^
../../third_party/fmtlib/src/include/fmt/base.h:524:32: note: read of dereferenced null pointer is not allowed in a constant expression
  524 |       size_ = __builtin_strlen(detail::narrow(s));
      |                                ^
../../third_party/fmtlib/src/include/fmt/base.h:2670:29: note: in call to 'basic_string_view(nullptr)'
 2670 |     FMT_CONSTEXPR auto sv = string_view(S());
      |                             ^~~~~~~~~~~~~~~~
../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:735:24: note: in call to 'fstring<const char *, 0>(tmpl)'
  735 |     out << fmt::format(tmpl, fmt::arg("name", name), fmt::arg("typelist", typelist));
      |                        ^~~~
../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:790:24: error: call to consteval function 'fmt::fstring<fmt::detail::named_arg<char, std::string>, fmt::detail::named_arg<char, std::string>, fmt::detail::named_arg<char, std::string>>::fstring<const char *, 0>' is not a constant expression
  790 |     out << fmt::format(tmpl, fmt::arg("name", name), fmt::arg("default_name", default_name),
      |                        ^
../../third_party/fmtlib/src/include/fmt/base.h:524:32: note: read of dereferenced null pointer is not allowed in a constant expression
  524 |       size_ = __builtin_strlen(detail::narrow(s));
      |                                ^
../../third_party/fmtlib/src/include/fmt/base.h:2670:29: note: in call to 'basic_string_view(nullptr)'
 2670 |     FMT_CONSTEXPR auto sv = string_view(S());
      |                             ^~~~~~~~~~~~~~~~
../../third_party/android/platform/system/tools/aidl/aidl_to_cpp_common.cpp:790:24: note: in call to 'fstring<const char *, 0>(tmpl)'
  790 |     out << fmt::format(tmpl, fmt::arg("name", name), fmt::arg("default_name", default_name),
      |                        ^~~~
5 errors generated.
zygoloid commented 2 weeks ago

I guess we should be using sv rather than S() after all.

zeroomega commented 2 weeks ago

I guess we should be using sv rather than S() after all.

You mean FMT_CONSTEXPR auto sv = string_view(s);? It fails as well.

zygoloid commented 2 weeks ago

You would also need to remove the FMT_CONSTEXPR. If that doesn't work, you'll need to share the error messages you're seeing :)

zeroomega commented 2 weeks ago

With diff like this:

diff --git a/include/fmt/base.h b/include/fmt/base.h
index f91f8e8..ebb8fdf 100644
--- a/include/fmt/base.h
+++ b/include/fmt/base.h
@@ -2667,7 +2667,7 @@ template <typename... T> struct fstring {
   template <typename S,
             FMT_ENABLE_IF(std::is_convertible<const S&, string_view>::value)>
   FMT_CONSTEVAL FMT_ALWAYS_INLINE fstring(const S& s) : str(s) {
-    FMT_CONSTEXPR auto sv = string_view(S());
+    auto sv = string_view(s);
     if (FMT_USE_CONSTEVAL)
       detail::parse_format_string<char>(sv, checker(sv, arg_pack()));
 #ifdef FMT_ENFORCE_COMPILE_STRING
diff --git a/include/fmt/format.h b/include/fmt/format.h
index 4ad1eff..52af87b 100644
--- a/include/fmt/format.h
+++ b/include/fmt/format.h
@@ -2674,7 +2674,7 @@ class bigint {

   FMT_CONSTEXPR auto get_bigit(int i) const -> bigit {
     return i >= exp_ && i < num_bigits() ? bigits_[i - exp_] : 0;
-  };
+  }

   FMT_CONSTEXPR void subtract_bigits(int index, bigit other, bigit& borrow) {
     auto result = double_bigit(bigits_[index]) - other - borrow;

The build passed.

zeroomega commented 2 weeks ago

Uploaded PR https://github.com/fmtlib/fmt/pull/4187