Repository: dtolnay/cxx (language: rust)

Issue:
Avoiding undefined behaviour in rust::Str/String on construction
rust::Str and rust::String constructors accept pointers and eventually call into slice::from_raw_parts() through cxxbridge04$str$valid() and cxxbridge04$string$from() from what I can gather.

Providing a nullptr to slice::from_raw_parts() is undefined behaviour, afaik.

We should probably add nullptr handling in the cases where it can happen, i.e. ptr/len constructor and ptr constructor. The std::string constructor seems save as it returns a valid pointer in the empty string case, see https://en.cppreference.com/w/cpp/string/basic_string/data


Discussion / hints:
Providing a `nullptr` to the `const char*` constructor will likely result in a crash on most platforms as it unconditionally calls `strlen()`.

I guess there could be a discussion if a plain `const char*` constructor should be provided in the first place, or if clients should always call the ptr/len constructor.
I would just say nullptr is not a legal input to any of the following constructors. I don't necessarily see it as something we need to check.
https://github.com/dtolnay/cxx/blob/4396abfafc69d5fe65fb10a5393a001d700a5043/include/cxx.h#L62-L63
https://github.com/dtolnay/cxx/blob/4396abfafc69d5fe65fb10a5393a001d700a5043/include/cxx.h#L32-L33
Sounds like a reasonable approach. Worth documenting?
I got a chance to look into how std::string treats these cases. As far as I have been able to tell, `std::string(nullptr)` and `std::string(nullptr, >0)` are always UB but `std::string(nullptr, 0)` is actually fine. The aim of the cxx builtin bindings has always been to expose an idiomatic API to the other language, so cutting down differences between our string's API and std::string's is a useful design guide, to the extent that std::string is idiomatic in C++.

For `rust::Str(nullptr, 0)` and `rust::String(nullptr, 0)` we'd be able to choose any of:

- UB (as currently)
- terminate
- valid empty string (by bumping ptr up by 1)

@adetaylor I'm curious what you would consider most appropriate for your use case.
My choice would be to terminate, but then I'm in a security team, so I _always_ want to crash explosively as soon as possible, before there's any chance of somebody exploiting the UB. My opinion might not be representative :)

Fix patch:
diff --git a/src/cxx.cc b/src/cxx.cc
index 715e0ce17..cbda8b944 100644
--- a/src/cxx.cc
+++ b/src/cxx.cc
@@ -57,12 +57,23 @@ String::String(String &&other) noexcept {
 
 String::~String() noexcept { cxxbridge05$string$drop(this); }
 
-String::String(const std::string &s) : String(s.data(), s.length()) {}
+String::String(const std::string &s) {
+  if (!cxxbridge05$string$from(this, s.data(), s.length())) {
+    panic<std::invalid_argument>("data for rust::String is not utf-8");
+  }
+}
 
-String::String(const char *s) : String(s, std::strlen(s)) {}
+String::String(const char *s) {
+  if (!cxxbridge05$string$from(this, s, std::strlen(s))) {
+    panic<std::invalid_argument>("data for rust::String is not utf-8");
+  }
+}
 
 String::String(const char *s, size_t len) {
-  if (!cxxbridge05$string$from(this, s, len)) {
+  if (!cxxbridge05$string$from(
+          this,
+          s == nullptr && len == 0 ? reinterpret_cast<const char *>(1) : s,
+          len)) {
     panic<std::invalid_argument>("data for rust::String is not utf-8");
   }
 }
@@ -104,15 +115,26 @@ std::ostream &operator<<(std::ostream &os, const String &s) {
   return os;
 }
 
-Str::Str() noexcept : repr(Repr{reinterpret_cast<const char *>(this), 0}) {}
+Str::Str() noexcept : repr(Repr{reinterpret_cast<const char *>(1), 0}) {}
 
 Str::Str(const Str &) noexcept = default;
 
-Str::Str(const std::string &s) : Str(s.data(), s.length()) {}
+Str::Str(const std::string &s) : repr(Repr{s.data(), s.length()}) {
+  if (!cxxbridge05$str$valid(this->repr.ptr, this->repr.len)) {
+    panic<std::invalid_argument>("data for rust::Str is not utf-8");
+  }
+}
 
-Str::Str(const char *s) : Str(s, std::strlen(s)) {}
+Str::Str(const char *s) : repr(Repr{s, std::strlen(s)}) {
+  if (!cxxbridge05$str$valid(this->repr.ptr, this->repr.len)) {
+    panic<std::invalid_argument>("data for rust::Str is not utf-8");
+  }
+}
 
-Str::Str(const char *s, size_t len) : repr(Repr{s, len}) {
+Str::Str(const char *s, size_t len)
+    : repr(
+          Repr{s == nullptr && len == 0 ? reinterpret_cast<const char *>(1) : s,
+               len}) {
   if (!cxxbridge05$str$valid(this->repr.ptr, this->repr.len)) {
     panic<std::invalid_argument>("data for rust::Str is not utf-8");
   }


Test patch: