src: unify implementations of Utf8Value etc.

Unify the common code of `Utf8Value`, `TwoByteValue`, `BufferValue`
and `StringBytes::InlineDecoder` into one class. Always make the
result zero-terminated for the first three.

This fixes two problems in passing:

* When the conversion of the input value to String fails,
  make the buffer zero-terminated anyway. Previously, this would
  have resulted in possibly reading uninitialized data in multiple
  places in the code. An instance of that problem can be reproduced
  by running e.g.
  `valgrind node -e 'net.isIP({ toString() { throw Error() } })'`.
* Previously, `BufferValue` copied one byte too much from the source,
  possibly resulting in an out-of-bounds memory access.
  This can be reproduced by running e.g.
  `valgrind node -e \
    'fs.openSync(Buffer.from("node".repeat(8192)), "r")'`.

Further minor changes:
* This lifts the `out()` method of `StringBytes::InlineDecoder`
  to the common class so that it can be used when using the
  overloaded `operator*` does not seem appropiate.
* Hopefully clearer variable names.
* Add checks to make sure the length of the data does not exceed
  the allocated storage size, including the possible null terminator.

PR-URL: #6357
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>

Author: addaleax

src/string_bytes.h

@@ -7,22 +7,14 @@
 #include "node.h"
 #include "env.h"
 #include "env-inl.h"
+#include "util.h"
 
 namespace node {
 
 class StringBytes {
  public:
-  class InlineDecoder {
+  class InlineDecoder : public MaybeStackBuffer<char> {
    public:
-    InlineDecoder() : out_(nullptr) {
-    }
-
-    ~InlineDecoder() {
-      if (out_ != out_st_)
-        delete[] out_;
-      out_ = nullptr;
-    }
-
     inline bool Decode(Environment* env,
                        v8::Local<v8::String> string,
                        v8::Local<v8::Value> encoding,
@@ -33,28 +25,22 @@ class StringBytes {
         return false;
       }
 
-      size_t buflen = StringBytes::StorageSize(env->isolate(), string, enc);
-      if (buflen > sizeof(out_st_))
-        out_ = new char[buflen];
-      else
-        out_ = out_st_;
-      size_ = StringBytes::Write(env->isolate(),
-                                 out_,
-                                 buflen,
-                                 string,
-                                 enc);
+      const size_t storage = StringBytes::StorageSize(env->isolate(),
+                                                      string,
+                                                      enc);
+      AllocateSufficientStorage(storage);
+      const size_t length = StringBytes::Write(env->isolate(),
+                                               out(),
+                                               storage,
+                                               string,
+                                               enc);
+
+      // No zero terminator is included when using this method.
+      SetLength(length);
       return true;
     }
 
-    inline const char* out() const { return out_; }
-    inline size_t size() const { return size_; }
-
-   private:
-    static const int kStorageSize = 1024;
-
-    char out_st_[kStorageSize];
-    char* out_;
-    size_t size_;
+    inline size_t size() const { return length(); }
   };
 
   // Does the string match the encoding? Quick but non-exhaustive.

src/util.cc

@@ -1,32 +1,78 @@
 #include "util.h"
+#include "node_buffer.h"
 #include "string_bytes.h"
 
 namespace node {
 
-Utf8Value::Utf8Value(v8::Isolate* isolate, v8::Local<v8::Value> value)
-    : length_(0), str_(str_st_) {
-  // Make sure result is always zero-terminated, even if conversion to string
-  // fails.
-  str_st_[0] = '\0';
+using v8::Isolate;
+using v8::Local;
+using v8::String;
+using v8::Value;
 
+template <typename T>
+static void MakeUtf8String(Isolate* isolate,
+                           Local<Value> value,
+                           T* target) {
+  Local<String> string = value->ToString(isolate);
+  if (string.IsEmpty())
+    return;
+
+  const size_t storage = StringBytes::StorageSize(isolate, string, UTF8) + 1;
+  target->AllocateSufficientStorage(storage);
+  const int flags =
+      String::NO_NULL_TERMINATION | String::REPLACE_INVALID_UTF8;
+  const int length = string->WriteUtf8(target->out(), storage, 0, flags);
+  target->SetLengthAndZeroTerminate(length);
+}
+
+Utf8Value::Utf8Value(Isolate* isolate, Local<Value> value) {
   if (value.IsEmpty())
     return;
 
+  MakeUtf8String(isolate, value, this);
+}
+
+
+TwoByteValue::TwoByteValue(Isolate* isolate, Local<Value> value) {
+  if (value.IsEmpty()) {
+    return;
+  }
+
   v8::Local<v8::String> string = value->ToString(isolate);
   if (string.IsEmpty())
     return;
 
   // Allocate enough space to include the null terminator
-  size_t len = StringBytes::StorageSize(isolate, string, UTF8) + 1;
-  if (len > sizeof(str_st_)) {
-    str_ = static_cast<char*>(malloc(len));
-    CHECK_NE(str_, nullptr);
-  }
+  const size_t storage = string->Length() + 1;
+  AllocateSufficientStorage(storage);
 
   const int flags =
-      v8::String::NO_NULL_TERMINATION | v8::String::REPLACE_INVALID_UTF8;
-  length_ = string->WriteUtf8(str_, len, 0, flags);
-  str_[length_] = '\0';
+      String::NO_NULL_TERMINATION | String::REPLACE_INVALID_UTF8;
+  const int length = string->Write(out(), 0, storage, flags);
+  SetLengthAndZeroTerminate(length);
+}
+
+BufferValue::BufferValue(Isolate* isolate, Local<Value> value) {
+  // Slightly different take on Utf8Value. If value is a String,
+  // it will return a Utf8 encoded string. If value is a Buffer,
+  // it will copy the data out of the Buffer as is.
+  if (value.IsEmpty()) {
+    // Dereferencing this object will return nullptr.
+    Invalidate();
+    return;
+  }
+
+  if (value->IsString()) {
+    MakeUtf8String(isolate, value, this);
+  } else if (Buffer::HasInstance(value)) {
+    const size_t len = Buffer::Length(value);
+    // Leave place for the terminating '\0' byte.
+    AllocateSufficientStorage(len + 1);
+    memcpy(out(), Buffer::Data(value), len);
+    SetLengthAndZeroTerminate(len);
+  } else {
+    Invalidate();
+  }
 }
 
 }  // namespace node

src/util.h

@@ -184,31 +184,102 @@ inline char ToLower(char c);
 // strcasecmp() is locale-sensitive.  Use StringEqualNoCase() instead.
 inline bool StringEqualNoCase(const char* a, const char* b);
 
-class Utf8Value {
+// Allocates an array of member type T. For up to kStackStorageSize items,
+// the stack is used, otherwise malloc().
+template <typename T, size_t kStackStorageSize = 1024>
+class MaybeStackBuffer {
   public:
-    explicit Utf8Value(v8::Isolate* isolate, v8::Local<v8::Value> value);
+    const T* out() const {
+      return buf_;
+    }
 
-    ~Utf8Value() {
-      if (str_ != str_st_)
-        free(str_);
+    T* out() {
+      return buf_;
     }
 
-    char* operator*() {
-      return str_;
-    };
+    // operator* for compatibility with `v8::String::(Utf8)Value`
+    T* operator*() {
+      return buf_;
+    }
 
-    const char* operator*() const {
-      return str_;
-    };
+    const T* operator*() const {
+      return buf_;
+    }
 
     size_t length() const {
       return length_;
-    };
+    }
+
+    // Call to make sure enough space for `storage` entries is available.
+    // There can only be 1 call to AllocateSufficientStorage or Invalidate
+    // per instance.
+    void AllocateSufficientStorage(size_t storage) {
+      if (storage <= kStackStorageSize) {
+        buf_ = buf_st_;
+      } else {
+        // Guard against overflow.
+        CHECK_LE(storage, sizeof(T) * storage);
+
+        buf_ = static_cast<T*>(malloc(sizeof(T) * storage));
+        CHECK_NE(buf_, nullptr);
+      }
+
+      // Remember how much was allocated to check against that in SetLength().
+      length_ = storage;
+    }
+
+    void SetLength(size_t length) {
+      // length_ stores how much memory was allocated.
+      CHECK_LE(length, length_);
+      length_ = length;
+    }
+
+    void SetLengthAndZeroTerminate(size_t length) {
+      // length_ stores how much memory was allocated.
+      CHECK_LE(length + 1, length_);
+      SetLength(length);
+
+      // T() is 0 for integer types, nullptr for pointers, etc.
+      buf_[length] = T();
+    }
+
+    // Make derefencing this object return nullptr.
+    // Calling this is mutually exclusive with calling
+    // AllocateSufficientStorage.
+    void Invalidate() {
+      CHECK_EQ(buf_, buf_st_);
+      length_ = 0;
+      buf_ = nullptr;
+    }
 
+    MaybeStackBuffer() : length_(0), buf_(buf_st_) {
+      // Default to a zero-length, null-terminated buffer.
+      buf_[0] = T();
+    }
+
+    ~MaybeStackBuffer() {
+      if (buf_ != buf_st_)
+        free(buf_);
+    }
   private:
     size_t length_;
-    char* str_;
-    char str_st_[1024];
+    T* buf_;
+    T buf_st_[kStackStorageSize];
+};
+
+class Utf8Value : public MaybeStackBuffer<char> {
+  public:
+    explicit Utf8Value(v8::Isolate* isolate, v8::Local<v8::Value> value);
+};
+
+class TwoByteValue : public MaybeStackBuffer<uint16_t> {
+  public:
+    explicit TwoByteValue(v8::Isolate* isolate, v8::Local<v8::Value> value);
+};
+
+class BufferValue : public MaybeStackBuffer<char> {
+  public:
+    explicit BufferValue(v8::Isolate* isolate, v8::Local<v8::Value> value);
 };
 
 }  // namespace node