src: fix fs.write() externalized string handling

* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: https://github.com/nodejs/node/issues/18146
PR-URL: https://github.com/nodejs/node/pull/18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

3 files changed, 61 insertions, 108 deletions

diff --git a/src/node_file.cc b/src/node_file.cc
index c721c4ba88..e6b1116e99 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -39,6 +39,7 @@
 # include <io.h>
 #endif
 
+#include <memory>
 #include <vector>
 
 namespace node {
@@ -1019,40 +1020,53 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
 
   CHECK(args[0]->IsInt32());
 
+  std::unique_ptr<char[]> delete_on_return;
   Local<Value> req;
-  Local<Value> string = args[1];
+  Local<Value> value = args[1];
   int fd = args[0]->Int32Value();
   char* buf = nullptr;
-  int64_t pos;
   size_t len;
+  const int64_t pos = GET_OFFSET(args[2]);
+  const auto enc = ParseEncoding(env->isolate(), args[3], UTF8);
+  const auto is_async = args[4]->IsObject();
+
+  // Avoid copying the string when it is externalized but only when:
+  // 1. The target encoding is compatible with the string's encoding, and
+  // 2. The write is synchronous, otherwise the string might get neutered
+  //    while the request is in flight, and
+  // 3. For UCS2, when the host system is little-endian.  Big-endian systems
+  //    need to call StringBytes::Write() to ensure proper byte swapping.
+  // The const_casts are conceptually sound: memory is read but not written.
+  if (!is_async && value->IsString()) {
+    auto string = value.As<String>();
+    if ((enc == ASCII || enc == LATIN1) && string->IsExternalOneByte()) {
+      auto ext = string->GetExternalOneByteStringResource();
+      buf = const_cast<char*>(ext->data());
+      len = ext->length();
+    } else if (enc == UCS2 && IsLittleEndian() && string->IsExternal()) {
+      auto ext = string->GetExternalStringResource();
+      buf = reinterpret_cast<char*>(const_cast<uint16_t*>(ext->data()));
+      len = ext->length() * sizeof(*ext->data());
+    }
+  }
 
-  // will assign buf and len if string was external
-  if (!StringBytes::GetExternalParts(string,
-                                     const_cast<const char**>(&buf),
-                                     &len)) {
-    enum encoding enc = ParseEncoding(env->isolate(), args[3], UTF8);
-    len = StringBytes::StorageSize(env->isolate(), string, enc);
+  if (buf == nullptr) {
+    len = StringBytes::StorageSize(env->isolate(), value, enc);
     buf = new char[len];
+    // SYNC_CALL returns on error.  Make sure to always free the memory.
+    if (!is_async) delete_on_return.reset(buf);
     // StorageSize may return too large a char, so correct the actual length
     // by the write size
     len = StringBytes::Write(env->isolate(), buf, len, args[1], enc);
   }
-  pos = GET_OFFSET(args[2]);
 
-  uv_buf_t uvbuf = uv_buf_init(const_cast<char*>(buf), len);
+  uv_buf_t uvbuf = uv_buf_init(buf, len);
 
-  if (args[4]->IsObject()) {
+  if (is_async) {
     CHECK_EQ(args.Length(), 5);
     AsyncCall(env, args, "write", UTF8, AfterInteger,
               uv_fs_write, fd, &uvbuf, 1, pos);
   } else {
-    // SYNC_CALL returns on error.  Make sure to always free the memory.
-    struct Delete {
-      inline explicit Delete(char* pointer) : pointer_(pointer) {}
-      inline ~Delete() { delete[] pointer_; }
-      char* const pointer_;
-    };
-    Delete delete_on_return(buf);
     SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos)
     return args.GetReturnValue().Set(SYNC_RESULT);
   }
diff --git a/src/string_bytes.cc b/src/string_bytes.cc
index 60e88c4588..d7c2ef0c52 100644
--- a/src/string_bytes.cc
+++ b/src/string_bytes.cc
@@ -27,6 +27,8 @@
 
 #include <limits.h>
 #include <string.h>  // memcpy
+
+#include <algorithm>
 #include <vector>
 
 // When creating strings >= this length v8's gc spins up and consumes
@@ -269,39 +271,6 @@ static size_t hex_decode(char* buf,
 }
 
 
-bool StringBytes::GetExternalParts(Local<Value> val,
-                                   const char** data,
-                                   size_t* len) {
-  if (Buffer::HasInstance(val)) {
-    *data = Buffer::Data(val);
-    *len = Buffer::Length(val);
-    return true;
-  }
-
-  if (!val->IsString())
-    return false;
-
-  Local<String> str = val.As<String>();
-
-  if (str->IsExternalOneByte()) {
-    const String::ExternalOneByteStringResource* ext;
-    ext = str->GetExternalOneByteStringResource();
-    *data = ext->data();
-    *len = ext->length();
-    return true;
-
-  } else if (str->IsExternal()) {
-    const String::ExternalStringResource* ext;
-    ext = str->GetExternalStringResource();
-    *data = reinterpret_cast<const char*>(ext->data());
-    *len = ext->length() * sizeof(*ext->data());
-    return true;
-  }
-
-  return false;
-}
-
-
 size_t StringBytes::WriteUCS2(char* buf,
                               size_t buflen,
                               Local<String> str,
@@ -347,17 +316,15 @@ size_t StringBytes::Write(Isolate* isolate,
                           enum encoding encoding,
                           int* chars_written) {
   HandleScope scope(isolate);
-  const char* data = nullptr;
-  size_t nbytes = 0;
-  const bool is_extern = GetExternalParts(val, &data, &nbytes);
-  const size_t external_nbytes = nbytes;
+  size_t nbytes;
+  int nchars;
+
+  if (chars_written == nullptr)
+    chars_written = &nchars;
 
   CHECK(val->IsString() == true);
   Local<String> str = val.As<String>();
 
-  if (nbytes > buflen)
-    nbytes = buflen;
-
   int flags = String::HINT_MANY_WRITES_EXPECTED |
               String::NO_NULL_TERMINATION |
               String::REPLACE_INVALID_UTF8;
@@ -365,14 +332,15 @@ size_t StringBytes::Write(Isolate* isolate,
   switch (encoding) {
     case ASCII:
     case LATIN1:
-      if (is_extern && str->IsOneByte()) {
-        memcpy(buf, data, nbytes);
+      if (str->IsExternalOneByte()) {
+        auto ext = str->GetExternalOneByteStringResource();
+        nbytes = std::min(buflen, ext->length());
+        memcpy(buf, ext->data(), nbytes);
       } else {
         uint8_t* const dst = reinterpret_cast<uint8_t*>(buf);
         nbytes = str->WriteOneByte(dst, 0, buflen, flags);
       }
-      if (chars_written != nullptr)
-        *chars_written = nbytes;
+      *chars_written = nbytes;
       break;
 
     case BUFFER:
@@ -383,14 +351,8 @@ size_t StringBytes::Write(Isolate* isolate,
     case UCS2: {
       size_t nchars;
 
-      if (is_extern && !str->IsOneByte()) {
-        memcpy(buf, data, nbytes);
-        nchars = nbytes / sizeof(uint16_t);
-      } else {
-        nbytes = WriteUCS2(buf, buflen, str, flags, &nchars);
-      }
-      if (chars_written != nullptr)
-        *chars_written = nchars;
+      nbytes = WriteUCS2(buf, buflen, str, flags, &nchars);
+      *chars_written = static_cast<int>(nchars);
 
       // Node's "ucs2" encoding wants LE character data stored in
       // the Buffer, so we need to reorder on BE platforms.  See
@@ -403,27 +365,25 @@ size_t StringBytes::Write(Isolate* isolate,
     }
 
     case BASE64:
-      if (is_extern) {
-        nbytes = base64_decode(buf, buflen, data, external_nbytes);
+      if (str->IsExternalOneByte()) {
+        auto ext = str->GetExternalOneByteStringResource();
+        nbytes = base64_decode(buf, buflen, ext->data(), ext->length());
       } else {
         String::Value value(str);
         nbytes = base64_decode(buf, buflen, *value, value.length());
       }
-      if (chars_written != nullptr) {
-        *chars_written = nbytes;
-      }
+      *chars_written = nbytes;
       break;
 
     case HEX:
-      if (is_extern) {
-        nbytes = hex_decode(buf, buflen, data, external_nbytes);
+      if (str->IsExternalOneByte()) {
+        auto ext = str->GetExternalOneByteStringResource();
+        nbytes = hex_decode(buf, buflen, ext->data(), ext->length());
       } else {
         String::Value value(str);
         nbytes = hex_decode(buf, buflen, *value, value.length());
       }
-      if (chars_written != nullptr) {
-        *chars_written = nbytes;
-      }
+      *chars_written = nbytes;
       break;
 
     default:
@@ -500,49 +460,34 @@ size_t StringBytes::Size(Isolate* isolate,
                          Local<Value> val,
                          enum encoding encoding) {
   HandleScope scope(isolate);
-  size_t data_size = 0;
-  bool is_buffer = Buffer::HasInstance(val);
 
-  if (is_buffer && (encoding == BUFFER || encoding == LATIN1))
+  if (Buffer::HasInstance(val) && (encoding == BUFFER || encoding == LATIN1))
     return Buffer::Length(val);
 
-  const char* data;
-  if (GetExternalParts(val, &data, &data_size))
-    return data_size;
-
   Local<String> str = val->ToString(isolate);
 
   switch (encoding) {
     case ASCII:
     case LATIN1:
-      data_size = str->Length();
-      break;
+      return str->Length();
 
     case BUFFER:
     case UTF8:
-      data_size = str->Utf8Length();
-      break;
+      return str->Utf8Length();
 
     case UCS2:
-      data_size = str->Length() * sizeof(uint16_t);
-      break;
+      return str->Length() * sizeof(uint16_t);
 
     case BASE64: {
       String::Value value(str);
-      data_size = base64_decoded_size(*value, value.length());
-      break;
+      return base64_decoded_size(*value, value.length());
     }
 
     case HEX:
-      data_size = str->Length() / 2;
-      break;
-
-    default:
-      CHECK(0 && "unknown encoding");
-      break;
+      return str->Length() / 2;
   }
 
-  return data_size;
+  UNREACHABLE();
 }
 
 
diff --git a/src/string_bytes.h b/src/string_bytes.h
index 17bbd80c0a..7a70f06f63 100644
--- a/src/string_bytes.h
+++ b/src/string_bytes.h
@@ -81,12 +81,6 @@ class StringBytes {
                      v8::Local<v8::Value> val,
                      enum encoding enc);
 
-  // If the string is external then assign external properties to data and len,
-  // then return true. If not return false.
-  static bool GetExternalParts(v8::Local<v8::Value> val,
-                               const char** data,
-                               size_t* len);
-
   // Write the bytes from the string or buffer into the char*
   // returns the number of bytes written, which will always be
   // <= buflen.  Use StorageSize/Size first to know how much