From 44a40325da4031f5a5470bec7b07fb8be5f9e99e Mon Sep 17 00:00:00 2001 From: Anna Henningsen 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 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- src/string_bytes.h | 44 +++++++------------ src/util.cc | 75 +++++++++++++++----------------- src/util.h | 123 ++++++++++++++++++++++++++++++++--------------------- 3 files changed, 123 insertions(+), 119 deletions(-) (limited to 'src') diff --git a/src/string_bytes.h b/src/string_bytes.h index 47a4255978..4b83b7c0f4 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 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, - char** dst, - const size_t size) { +template +static void MakeUtf8String(Isolate* isolate, + Local value, + T* target) { Local string = value->ToString(isolate); if (string.IsEmpty()) - return 0; - size_t len = StringBytes::StorageSize(isolate, string, UTF8) + 1; - if (len > size) { - *dst = static_cast(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) - : length_(0), str_(str_st_) { +Utf8Value::Utf8Value(Isolate* isolate, Local value) { if (value.IsEmpty()) return; - length_ = MakeUtf8String(isolate, value, &str_, sizeof(str_st_)); + + MakeUtf8String(isolate, value, this); } -TwoByteValue::TwoByteValue(Isolate* isolate, Local value) - : length_(0), str_(str_st_) { - if (value.IsEmpty()) +TwoByteValue::TwoByteValue(Isolate* isolate, Local value) { + if (value.IsEmpty()) { return; + } Local 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(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) - : str_(str_st_), fail_(true) { +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()) + 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(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(); } } diff --git a/src/util.h b/src/util.h index 2c4d8c6286..1f9f81cfd3 100644 --- a/src/util.h +++ b/src/util.h @@ -178,77 +178,102 @@ inline TypeName* Unwrap(v8::Local object); inline void SwapBytes(uint16_t* dst, const uint16_t* src, size_t buflen); -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_; - }; - - private: - size_t length_; - char* str_; - char str_st_[1024]; -}; + } -class TwoByteValue { - public: - explicit TwoByteValue(v8::Isolate* isolate, v8::Local value); + // 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; + } - ~TwoByteValue() { - if (str_ != str_st_) - free(str_); + void SetLength(size_t length) { + // length_ stores how much memory was allocated. + CHECK_LE(length, length_); + length_ = length; } - uint16_t* operator*() { - return str_; - }; + void SetLengthAndZeroTerminate(size_t length) { + // length_ stores how much memory was allocated. + CHECK_LE(length + 1, length_); + SetLength(length); - const uint16_t* operator*() const { - return str_; - }; + // T() is 0 for integer types, nullptr for pointers, etc. + buf_[length] = T(); + } - size_t length() const { - return length_; - }; + // 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_; - uint16_t* str_; - uint16_t str_st_[1024]; + T* buf_; + T buf_st_[kStackStorageSize]; }; -class BufferValue { +class Utf8Value : public MaybeStackBuffer { public: - explicit BufferValue(v8::Isolate* isolate, v8::Local value); - - ~BufferValue() { - if (str_ != str_st_) - free(str_); - } + explicit Utf8Value(v8::Isolate* isolate, v8::Local value); +}; - const char* operator*() const { - return fail_ ? nullptr : str_; - }; +class TwoByteValue : public MaybeStackBuffer { + public: + explicit TwoByteValue(v8::Isolate* isolate, v8::Local value); +}; - private: - char* str_; - char str_st_[1024]; - bool fail_; +class BufferValue : public MaybeStackBuffer { + public: + explicit BufferValue(v8::Isolate* isolate, v8::Local value); }; } // namespace node -- cgit v1.2.3