Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit fa9d82d

Browse filesBrowse files
addaleaxFishrock123
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
Copy full SHA for fa9d82d

File tree

Expand file treeCollapse file tree

3 files changed

+123
-119
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+123
-119
lines changed
Open diff view settings
Collapse file

‎src/string_bytes.h‎

Copy file name to clipboardExpand all lines: src/string_bytes.h
+15-29Lines 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.
Collapse file

‎src/util.cc‎

Copy file name to clipboardExpand all lines: src/util.cc
+34-41Lines 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

Collapse file

‎src/util.h‎

Copy file name to clipboardExpand all lines: src/util.h
+74-49Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.