From 44a40325da4031f5a5470bec7b07fb8be5f9e99e Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 23 Apr 2016 22:04:30 +0200
Subject: 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: https://github.com/nodejs/node/pull/6357
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
---
 src/util.cc | 75 ++++++++++++++++++++++++++++---------------------------------
 1 file changed, 34 insertions(+), 41 deletions(-)

(limited to 'src/util.cc')

diff --git a/src/util.cc b/src/util.cc
index 0de8321e3f..7ce99d5c76 100644
--- a/src/util.cc
+++ b/src/util.cc
@@ -10,76 +10,69 @@ using v8::Local;
 using v8::String;
 using v8::Value;
 
-static int MakeUtf8String(Isolate* isolate,
-                          Local<Value> value,
-                          char** dst,
-                          const size_t size) {
+template <typename T>
+static void MakeUtf8String(Isolate* isolate,
+                           Local<Value> value,
+                           T* target) {
   Local<String> string = value->ToString(isolate);
   if (string.IsEmpty())
-    return 0;
-  size_t len = StringBytes::StorageSize(isolate, string, UTF8) + 1;
-  if (len > size) {
-    *dst = static_cast<char*>(malloc(len));
-    CHECK_NE(*dst, nullptr);
-  }
+    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(*dst, len, 0, flags);
-  (*dst)[length] = '\0';
-  return length;
+  const int length = string->WriteUtf8(target->out(), storage, 0, flags);
+  target->SetLengthAndZeroTerminate(length);
 }
 
-Utf8Value::Utf8Value(Isolate* isolate, Local<Value> value)
-    : length_(0), str_(str_st_) {
+Utf8Value::Utf8Value(Isolate* isolate, Local<Value> value) {
   if (value.IsEmpty())
     return;
-  length_ = MakeUtf8String(isolate, value, &str_, sizeof(str_st_));
+
+  MakeUtf8String(isolate, value, this);
 }
 
 
-TwoByteValue::TwoByteValue(Isolate* isolate, Local<Value> value)
-    : length_(0), str_(str_st_) {
-  if (value.IsEmpty())
+TwoByteValue::TwoByteValue(Isolate* isolate, Local<Value> value) {
+  if (value.IsEmpty()) {
     return;
+  }
 
   Local<String> string = value->ToString(isolate);
   if (string.IsEmpty())
     return;
 
   // Allocate enough space to include the null terminator
-  size_t len =
-      StringBytes::StorageSize(isolate, string, UCS2) +
-      sizeof(uint16_t);
-  if (len > sizeof(str_st_)) {
-    str_ = static_cast<uint16_t*>(malloc(len));
-    CHECK_NE(str_, nullptr);
-  }
+  const size_t storage = string->Length() + 1;
+  AllocateSufficientStorage(storage);
 
   const int flags =
       String::NO_NULL_TERMINATION | String::REPLACE_INVALID_UTF8;
-  length_ = string->Write(str_, 0, len, flags);
-  str_[length_] = '\0';
+  const int length = string->Write(out(), 0, storage, flags);
+  SetLengthAndZeroTerminate(length);
 }
 
-BufferValue::BufferValue(Isolate* isolate, Local<Value> value)
-    : str_(str_st_), fail_(true) {
+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())
+  if (value.IsEmpty()) {
+    // Dereferencing this object will return nullptr.
+    Invalidate();
     return;
+  }
+
   if (value->IsString()) {
-    MakeUtf8String(isolate, value, &str_, sizeof(str_st_));
-    fail_ = false;
+    MakeUtf8String(isolate, value, this);
   } else if (Buffer::HasInstance(value)) {
-    size_t len = Buffer::Length(value) + 1;
-    if (len > sizeof(str_st_)) {
-      str_ = static_cast<char*>(malloc(len));
-      CHECK_NE(str_, nullptr);
-    }
-    memcpy(str_, Buffer::Data(value), len);
-    str_[len - 1] = '\0';
-    fail_ = false;
+    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();
   }
 }
 
-- 
cgit v1.2.3