FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: unify implementations of Utf8Value etc. · nodejs/node@fa9d82d · GitHub

/ node Public

Commit fa9d82d

Browse files
addaleax
authored and
Fishrock123
committed
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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
1 parent e62c42b commit fa9d82d

3 files changed

Lines changed: 123 additions & 119 deletions

File tree

‎src/string_bytes.h‎

Lines changed: 15 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -7,22 +7,14 @@
77
#include "node.h"
88
#include "env.h"
99
#include "env-inl.h"
10+
#include "util.h"
1011

1112
namespace node {
1213

1314
class StringBytes {
1415
public:
15-
class InlineDecoder {
16+
class InlineDecoder : public MaybeStackBuffer<char> {
1617
public:
17-
InlineDecoder() : out_(nullptr) {
18-
}
19-
20-
~InlineDecoder() {
21-
if (out_ != out_st_)
22-
delete[] out_;
23-
out_ = nullptr;
24-
}
25-
2618
inline bool Decode(Environment* env,
2719
v8::Local<v8::String> string,
2820
v8::Local<v8::Value> encoding,
@@ -33,28 +25,22 @@ class StringBytes {
3325
return false;
3426
}
3527

36-
size_t buflen = StringBytes::StorageSize(env->isolate(), string, enc);
37-
if (buflen > sizeof(out_st_))
38-
out_ = new char[buflen];
39-
else
40-
out_ = out_st_;
41-
size_ = StringBytes::Write(env->isolate(),
42-
out_,
43-
buflen,
44-
string,
45-
enc);
28+
const size_t storage = StringBytes::StorageSize(env->isolate(),
29+
string,
30+
enc);
31+
AllocateSufficientStorage(storage);
32+
const size_t length = StringBytes::Write(env->isolate(),
33+
out(),
34+
storage,
35+
string,
36+
enc);
37+
38+
// No zero terminator is included when using this method.
39+
SetLength(length);
4640
return true;
4741
}
4842

49-
inline const char* out() const { return out_; }
50-
inline size_t size() const { return size_; }
51-
52-
private:
53-
static const int kStorageSize = 1024;
54-
55-
char out_st_[kStorageSize];
56-
char* out_;
57-
size_t size_;
43+
inline size_t size() const { return length(); }
5844
};
5945

6046
// Does the string match the encoding? Quick but non-exhaustive.

‎src/util.cc‎

Lines changed: 34 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,76 +10,69 @@ using v8::Local;
1010
using v8::String;
1111
using v8::Value;
1212

13-
static int MakeUtf8String(Isolate* isolate,
14-
Local<Value> value,
15-
char** dst,
16-
const size_t size) {
13+
template <typename T>
14+
static void MakeUtf8String(Isolate* isolate,
15+
Local<Value> value,
16+
T* target) {
1717
Local<String> string = value->ToString(isolate);
1818
if (string.IsEmpty())
19-
return 0;
20-
size_t len = StringBytes::StorageSize(isolate, string, UTF8) + 1;
21-
if (len > size) {
22-
*dst = static_cast<char*>(malloc(len));
23-
CHECK_NE(*dst, nullptr);
24-
}
19+
return;
20+
21+
const size_t storage = StringBytes::StorageSize(isolate, string, UTF8) + 1;
22+
target->AllocateSufficientStorage(storage);
2523
const int flags =
2624
String::NO_NULL_TERMINATION | String::REPLACE_INVALID_UTF8;
27-
const int length = string->WriteUtf8(*dst, len, 0, flags);
28-
(*dst)[length] = '\0';
29-
return length;
25+
const int length = string->WriteUtf8(target->out(), storage, 0, flags);
26+
target->SetLengthAndZeroTerminate(length);
3027
}
3128

32-
Utf8Value::Utf8Value(Isolate* isolate, Local<Value> value)
33-
: length_(0), str_(str_st_) {
29+
Utf8Value::Utf8Value(Isolate* isolate, Local<Value> value) {
3430
if (value.IsEmpty())
3531
return;
36-
length_ = MakeUtf8String(isolate, value, &str_, sizeof(str_st_));
32+
33+
MakeUtf8String(isolate, value, this);
3734
}
3835

3936

40-
TwoByteValue::TwoByteValue(Isolate* isolate, Local<Value> value)
41-
: length_(0), str_(str_st_) {
42-
if (value.IsEmpty())
37+
TwoByteValue::TwoByteValue(Isolate* isolate, Local<Value> value) {
38+
if (value.IsEmpty()) {
4339
return;
40+
}
4441

4542
Local<String> string = value->ToString(isolate);
4643
if (string.IsEmpty())
4744
return;
4845

4946
// Allocate enough space to include the null terminator
50-
size_t len =
51-
StringBytes::StorageSize(isolate, string, UCS2) +
52-
sizeof(uint16_t);
53-
if (len > sizeof(str_st_)) {
54-
str_ = static_cast<uint16_t*>(malloc(len));
55-
CHECK_NE(str_, nullptr);
56-
}
47+
const size_t storage = string->Length() + 1;
48+
AllocateSufficientStorage(storage);
5749

5850
const int flags =
5951
String::NO_NULL_TERMINATION | String::REPLACE_INVALID_UTF8;
60-
length_ = string->Write(str_, 0, len, flags);
61-
str_[length_] = '\0';
52+
const int length = string->Write(out(), 0, storage, flags);
53+
SetLengthAndZeroTerminate(length);
6254
}
6355

64-
BufferValue::BufferValue(Isolate* isolate, Local<Value> value)
65-
: str_(str_st_), fail_(true) {
56+
BufferValue::BufferValue(Isolate* isolate, Local<Value> value) {
6657
// Slightly different take on Utf8Value. If value is a String,
6758
// it will return a Utf8 encoded string. If value is a Buffer,
6859
// it will copy the data out of the Buffer as is.
69-
if (value.IsEmpty())
60+
if (value.IsEmpty()) {
61+
// Dereferencing this object will return nullptr.
62+
Invalidate();
7063
return;
64+
}
65+
7166
if (value->IsString()) {
72-
MakeUtf8String(isolate, value, &str_, sizeof(str_st_));
73-
fail_ = false;
67+
MakeUtf8String(isolate, value, this);
7468
} else if (Buffer::HasInstance(value)) {
75-
size_t len = Buffer::Length(value) + 1;
76-
if (len > sizeof(str_st_)) {
77-
str_ = static_cast<char*>(malloc(len));
78-
CHECK_NE(str_, nullptr);
79-
}
80-
memcpy(str_, Buffer::Data(value), len);
81-
str_[len - 1] = '\0';
82-
fail_ = false;
69+
const size_t len = Buffer::Length(value);
70+
// Leave place for the terminating '\0' byte.
71+
AllocateSufficientStorage(len + 1);
72+
memcpy(out(), Buffer::Data(value), len);
73+
SetLengthAndZeroTerminate(len);
74+
} else {
75+
Invalidate();
8376
}
8477
}
8578

‎src/util.h‎

Lines changed: 74 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -178,77 +178,102 @@ inline TypeName* Unwrap(v8::Local<v8::Object> object);
178178

179179
inline void SwapBytes(uint16_t* dst, const uint16_t* src, size_t buflen);
180180

181-
class Utf8Value {
181+
// Allocates an array of member type T. For up to kStackStorageSize items,
182+
// the stack is used, otherwise malloc().
183+
template <typename T, size_t kStackStorageSize = 1024>
184+
class MaybeStackBuffer {
182185
public:
183-
explicit Utf8Value(v8::Isolate* isolate, v8::Local<v8::Value> value);
186+
const T* out() const {
187+
return buf_;
188+
}
184189

185-
~Utf8Value() {
186-
if (str_ != str_st_)
187-
free(str_);
190+
T* out() {
191+
return buf_;
188192
}
189193

190-
char* operator*() {
191-
return str_;
192-
};
194+
// operator* for compatibility with `v8::String::(Utf8)Value`
195+
T* operator*() {
196+
return buf_;
197+
}
193198

194-
const char* operator*() const {
195-
return str_;
196-
};
199+
const T* operator*() const {
200+
return buf_;
201+
}
197202

198203
size_t length() const {
199204
return length_;
200-
};
201-
202-
private:
203-
size_t length_;
204-
char* str_;
205-
char str_st_[1024];
206-
};
205+
}
207206

208-
class TwoByteValue {
209-
public:
210-
explicit TwoByteValue(v8::Isolate* isolate, v8::Local<v8::Value> value);
207+
// Call to make sure enough space for `storage` entries is available.
208+
// There can only be 1 call to AllocateSufficientStorage or Invalidate
209+
// per instance.
210+
void AllocateSufficientStorage(size_t storage) {
211+
if (storage <= kStackStorageSize) {
212+
buf_ = buf_st_;
213+
} else {
214+
// Guard against overflow.
215+
CHECK_LE(storage, sizeof(T) * storage);
216+
217+
buf_ = static_cast<T*>(malloc(sizeof(T) * storage));
218+
CHECK_NE(buf_, nullptr);
219+
}
220+
221+
// Remember how much was allocated to check against that in SetLength().
222+
length_ = storage;
223+
}
211224

212-
~TwoByteValue() {
213-
if (str_ != str_st_)
214-
free(str_);
225+
void SetLength(size_t length) {
226+
// length_ stores how much memory was allocated.
227+
CHECK_LE(length, length_);
228+
length_ = length;
215229
}
216230

217-
uint16_t* operator*() {
218-
return str_;
219-
};
231+
void SetLengthAndZeroTerminate(size_t length) {
232+
// length_ stores how much memory was allocated.
233+
CHECK_LE(length + 1, length_);
234+
SetLength(length);
220235

221-
const uint16_t* operator*() const {
222-
return str_;
223-
};
236+
// T() is 0 for integer types, nullptr for pointers, etc.
237+
buf_[length] = T();
238+
}
224239

225-
size_t length() const {
226-
return length_;
227-
};
240+
// Make derefencing this object return nullptr.
241+
// Calling this is mutually exclusive with calling
242+
// AllocateSufficientStorage.
243+
void Invalidate() {
244+
CHECK_EQ(buf_, buf_st_);
245+
length_ = 0;
246+
buf_ = nullptr;
247+
}
248+
249+
MaybeStackBuffer() : length_(0), buf_(buf_st_) {
250+
// Default to a zero-length, null-terminated buffer.
251+
buf_[0] = T();
252+
}
228253

254+
~MaybeStackBuffer() {
255+
if (buf_ != buf_st_)
256+
free(buf_);
257+
}
229258
private:
230259
size_t length_;
231-
uint16_t* str_;
232-
uint16_t str_st_[1024];
260+
T* buf_;
261+
T buf_st_[kStackStorageSize];
233262
};
234263

235-
class BufferValue {
264+
class Utf8Value : public MaybeStackBuffer<char> {
236265
public:
237-
explicit BufferValue(v8::Isolate* isolate, v8::Local<v8::Value> value);
238-
239-
~BufferValue() {
240-
if (str_ != str_st_)
241-
free(str_);
242-
}
266+
explicit Utf8Value(v8::Isolate* isolate, v8::Local<v8::Value> value);
267+
};
243268

244-
const char* operator*() const {
245-
return fail_ ? nullptr : str_;
246-
};
269+
class TwoByteValue : public MaybeStackBuffer<uint16_t> {
270+
public:
271+
explicit TwoByteValue(v8::Isolate* isolate, v8::Local<v8::Value> value);
272+
};
247273

248-
private:
249-
char* str_;
250-
char str_st_[1024];
251-
bool fail_;
274+
class BufferValue : public MaybeStackBuffer<char> {
275+
public:
276+
explicit BufferValue(v8::Isolate* isolate, v8::Local<v8::Value> value);
252277
};
253278

254279
} // namespace node

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL