From dda81b44b096e5dee604e2f3f6278f41afb40598 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 23 Apr 2016 22:04:30 +0200 Subject: [PATCH] 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 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- src/string_bytes.h | 44 +++++++-------------- src/util.cc | 72 +++++++++++++++++++++++++++------ src/util.h | 99 +++++++++++++++++++++++++++++++++++++++------- 3 files changed, 159 insertions(+), 56 deletions(-) diff --git a/src/string_bytes.h b/src/string_bytes.h index 79520d24705ac0..fa5a7ca697586f 100644 --- a/src/string_bytes.h +++ b/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 { public: - InlineDecoder() : out_(nullptr) { - } - - ~InlineDecoder() { - if (out_ != out_st_) - delete[] out_; - out_ = nullptr; - } - inline bool Decode(Environment* env, v8::Local string, v8::Local 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. diff --git a/src/util.cc b/src/util.cc index 858e4b396534b3..f597015331d7f7 100644 --- a/src/util.cc +++ b/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 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 +static void MakeUtf8String(Isolate* isolate, + Local value, + T* target) { + Local 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) { if (value.IsEmpty()) return; + MakeUtf8String(isolate, value, this); +} + + +TwoByteValue::TwoByteValue(Isolate* isolate, Local value) { + if (value.IsEmpty()) { + return; + } + v8::Local 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(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) { + // 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 diff --git a/src/util.h b/src/util.h index 671b98dd25b191..dce6c343b1b443 100644 --- a/src/util.h +++ b/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 +class MaybeStackBuffer { public: - explicit Utf8Value(v8::Isolate* isolate, v8::Local 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(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 { + public: + explicit Utf8Value(v8::Isolate* isolate, v8::Local value); +}; + +class TwoByteValue : public MaybeStackBuffer { + public: + explicit TwoByteValue(v8::Isolate* isolate, v8::Local value); +}; + +class BufferValue : public MaybeStackBuffer { + public: + explicit BufferValue(v8::Isolate* isolate, v8::Local value); }; } // namespace node