Implement LWG 3112 - system_error and filesystem_error constructors taking a string may not be able to meet their postconditions

[EricWF]

Summary

This patch addresses LWG 3112 by transforming embedded nulls in what_arg strings from \0 to \\0.

src/filesystem/filesystem_common.h

static string format_string_imp(const char* msg, ...) {
   ret = ::vsnprintf(&result[0], size_with_null, msg, args);
   _LIBCPP_ASSERT(static_cast<size_t>(ret) == (size_with_null - 1), "TODO");
 
   return result;
 }
 
 const char* unwrap(string const& s) { return s.c_str(); }
 const char* unwrap(path const& p) { return p.native().c_str(); }
+const char* unwrap(string_view sv) { return sv.data(); }
 template <class Arg>
 Arg const& unwrap(Arg const& a) {
   static_assert(!is_class<Arg>::value, "cannot pass class here");
   return a;
 }
 
 template <class... Args>
 string format_string(const char* fmt, Args const&... args) {

src/filesystem/operations.cpp

#else
   timeval tv;
   gettimeofday(&tv, 0);
   return time_point(__secs(tv.tv_sec) + __microsecs(tv.tv_usec));
 #endif // _LIBCPP_USE_CLOCK_GETTIME && CLOCK_REALTIME
 }
 
 filesystem_error::~filesystem_error() {}
 
+static bool contains_null(string_view sv) {
+  return sv.find('\0') != string_view::npos;
+}
+
+
+static string_view sanitize_path_for_exception(string_view p, string &buff) {
+  if (p.empty())
+    return "\"\"";
+  auto num_null = std::count(p.begin(), p.end(), '\0');
+  if (num_null == 0)
+    return p;
+
+  buff = {};
+  const size_t new_size = p.size() + num_null;
+  buff.__resize_default_init(new_size);
+  auto it = buff.begin();
+  for (auto ch : p) {
+    if (ch == '\0') {
+      *it++ = '\\';
+      *it++ = '0';
+    } else {
+      *it++ = ch;
+    }
+  }
+  return buff;
+}
+
 void filesystem_error::__create_what(int __num_paths) {
   const char* derived_what = system_error::what();
   __storage_->__what_ = [&]() -> string {
-    const char* p1 = path1().native().empty() ? "\"\"" : path1().c_str();
-    const char* p2 = path2().native().empty() ? "\"\"" : path2().c_str();
+    string p1_buff, p2_buff;
+    string_view p1 = sanitize_path_for_exception(path1().native(), p1_buff);
+    string_view p2 = sanitize_path_for_exception(path2().native(), p2_buff);
     switch (__num_paths) {
     default:
       return detail::format_string("filesystem error: %s", derived_what);
     case 1:
       return detail::format_string("filesystem error: %s [%s]", derived_what,
                                    p1);
     case 2:
       return detail::format_string("filesystem error: %s [%s] [%s]",

src/system_error.cpp

 string
 error_code::message() const
 {
     return __cat_->message(__val_);
 }
 
 // system_error
 
+static void escape_null(string &s) {
+  if (auto num_null = std::count(s.begin(), s.end(), '\0')) {
+    std::string new_s;
+    const size_t new_size = s.size() + num_null;
+    new_s.__resize_default_init(new_size);
+    auto it = new_s.begin();
+    for (auto ch : s) {
+      if (ch == '\0') {
+        *it++ = '\\';
+        *it++ = '0';
+      } else {
+       *it++ = ch;
+      }
+    }
+    s.swap(new_s);
+  }
+}
+
 string
 system_error::__init(const error_code& ec, string what_arg)
 {
+    escape_null(what_arg);
     if (ec)
     {
         if (!what_arg.empty())
             what_arg += ": ";
         what_arg += ec.message();
     }
     return what_arg;
 }

test/std/diagnostics/syserr/syserr.syserr/syserr.syserr.members/ctor_error_code_string.pass.cpp

 // system_error(error_code ec, const string& what_arg);
 
 // Test is slightly non-portable
 
 #include <system_error>
 #include <string>
 #include <cassert>
 
-int main()
-{
-    std::string what_arg("test message");
+#include "test_macros.h"
+
+std::string sanitize_null_like_libcxx(std::string s) {
+    size_t pos;
+    while ((pos = s.find('\0')) != std::string::npos)
+        s.replace(pos, 1, "\\0");
+    return s;
+}
+
+void do_test(std::string const& what_arg) {
     std::system_error se(make_error_code(std::errc::not_a_directory), what_arg);
     assert(se.code() == std::make_error_code(std::errc::not_a_directory));
     std::string what_message(se.what());
-    assert(what_message.find(what_arg) != std::string::npos);
+    assert(what_message.find(what_arg.c_str()) != std::string::npos);
     assert(what_message.find("Not a directory") != std::string::npos);
+
+    // Test libc++'s behavior in case of null. libc++ replaces each null in the
+    // 'what' argument with "\\0" instead.
+    LIBCPP_ASSERT(what_message.find(sanitize_null_like_libcxx(what_arg)) != std::string::npos);
+}
+
+int main()
+{
+    {
+        std::string what_arg("my message");
+        do_test(what_arg);
+    }
+    { // test with embedded null
+        const char what_msg[] = "test message\0message after null";
+        const size_t what_size = sizeof(what_msg) / sizeof(what_msg[0]);
+        std::string what_arg(what_msg, what_size-1);
+        assert(what_arg.back() == 'l');
+        do_test(what_arg);
+    }
 }

test/std/input.output/filesystems/class.filesystem_error/filesystem_error.members.pass.cpp

 // const path& path2() const noexcept;
 
 #include "filesystem_include.hpp"
 #include <type_traits>
 #include <cassert>
 
 #include "test_macros.h"
 
+std::string sanitize_null_like_libcxx(std::string s) {
+  size_t pos;
+  while ((pos = s.find('\0')) != std::string::npos)
+    s.replace(pos, 1, "\\0");
+  return s;
+}
+
+std::string string_with_null(std::string LHS, std::string RHS) {
+  std::string result = std::move(LHS);
+  auto old_size = result.size();
+  result += '\0';
+  assert(result.size() == old_size + 1);
+  result += RHS;
+  return result;
+}
 
 void test_constructors() {
   using namespace fs;
 
-  // The string returned by "filesystem_error::what() must contain runtime_error::what()
   const std::string what_arg = "Hello World";
-  const std::string what_contains = std::runtime_error(what_arg).what();
-  assert(what_contains.find(what_arg) != std::string::npos);
-  auto CheckWhat = [what_contains](filesystem_error const& e) {
-    std::string s = e.what();
-    assert(s.find(what_contains) != std::string::npos);
-  };
-
   std::error_code ec = std::make_error_code(std::errc::file_exists);
   const path p1("foo");
   const path p2("bar");
 
+  auto CheckWhat = [&](filesystem_error const& e) {
+    std::system_error se(e.code(), what_arg);
+    std::string what = e.what();
+    assert(what.find(what_arg.c_str()) != std::string::npos);
+    assert(what.find(se.what()) != std::string::npos);
+  };
+
   // filesystem_error(const string& what_arg, error_code ec);
   {
     ASSERT_NOT_NOEXCEPT(filesystem_error(what_arg, ec));
     filesystem_error e(what_arg, ec);
     CheckWhat(e);
     assert(e.code() == ec);
     assert(e.path1().empty() && e.path2().empty());
   }
// filesystem_error(const string& what_arg, const path&, const path&, error_code ec);
     filesystem_error e(what_arg, p1, p2, ec);
     CheckWhat(e);
     assert(e.code() == ec);
     assert(e.path1() == p1);
     assert(e.path2() == p2);
   }
 }
 
+
+void test_constructors_with_null() {
+  using namespace fs;
+
+  const std::string what_arg = string_with_null("Hello World", "After Null!");
+  std::error_code ec = std::make_error_code(std::errc::file_exists);
+  const path p1(string_with_null("foo", "after_null"));
+  const path p2(string_with_null("bar", "after_null"));
+
+  const std::string what_arg_sanitized = "Hello World\\0After Null!";
+  const std::string p1_sanitized = "foo\\0after_null";
+  const std::string p2_sanitized = "bar\\0after_null";
+  ((void)what_arg_sanitized); // Only used by libc++
+  ((void)p1_sanitized);
+  ((void)p2_sanitized);
+
+  auto CheckWhat = [&](filesystem_error const& e, int num_paths) {
+    ((void)num_paths);
+    std::system_error se(e.code(), what_arg);
+    std::string what = e.what();
+    assert(what.find(what_arg.c_str()) != std::string::npos);
+    assert(what.find(se.what()) != std::string::npos);
+    LIBCPP_ASSERT(what.find("After Null!") != std::string::npos);
+    LIBCPP_ASSERT(what.find(what_arg_sanitized) != std::string::npos);
+    if (num_paths == 1 || num_paths == 2) {
+      LIBCPP_ASSERT(what.find(p1_sanitized) != std::string::npos);
+    }
+    if (num_paths == 2) {
+      LIBCPP_ASSERT(what.find(p2_sanitized) != std::string::npos);
+    }
+  };
+
+  // filesystem_error(const string& what_arg, error_code ec);
+  {
+    filesystem_error e(what_arg, ec);
+    CheckWhat(e, 0);
+    assert(e.code() == ec);
+    assert(e.path1().empty() && e.path2().empty());
+  }
+  // filesystem_error(const string& what_arg, const path&, error_code ec);
+  {
+    ASSERT_NOT_NOEXCEPT(filesystem_error(what_arg, p1, ec));
+    filesystem_error e(what_arg, p1, ec);
+    CheckWhat(e, 1);
+    assert(e.code() == ec);
+    assert(e.path1() == p1);
+    assert(e.path2().empty());
+  }
+  // filesystem_error(const string& what_arg, const path&, const path&, error_code ec);
+  {
+    filesystem_error e(what_arg, p1, p2, ec);
+    CheckWhat(e, 2);
+    assert(e.code() == ec);
+    assert(e.path1() == p1);
+    assert(e.path2() == p2);
+  }
+}
+
 void test_signatures()
 {
   using namespace fs;
   const path p;
   std::error_code ec;
   const filesystem_error e("lala", ec);
   // const path& path1() const noexcept;
   {
// const char* what() const noexcept
     ASSERT_SAME_TYPE(const char*, decltype(e.what()));
     ASSERT_NOEXCEPT(e.what());
   }
 }
 
 int main() {
   static_assert(std::is_base_of<std::system_error, fs::filesystem_error>::value, "");
   test_constructors();
+  test_constructors_with_null();
   test_signatures();
 }

www/upcoming_meeting.html

 
 <h3>Comments about the issues</h3>
 <ul>
 <li>3012 - </li>
 <li>3040 - We already do this</li>
 <li>3077 - No changes required</li>
 <li>3087 - No changes required</li>
 <li>3101 - Patch: <a href="https://reviews.llvm.org/D57058">D57058</a></li>
-<li>3112 - Eric - can you take a look?</li>
+<li>3112 - Patch: <a href="https://reviews.llvm.org/D57312">D57312</a></li>
 <li>3119 - No changes required</li>
 <li>3133 - </li>
 <li>3144 - Patch: <a href="https://reviews.llvm.org/D57039">D57039</a></li>
 <li>3173 - We don't have ranges yet</li>
 <li>3179 - We don't have ranges yet</li>
 <li>3180 - We don't have ranges yet</li>
 <li>3182 - We don't have concepts yet</li>
 </ul>
 
 <p>Last Updated: 21-Jan-2019</p>
 </div>
 </body>
 </html>