Skip to content

Commit 663b343

Browse files
committed
net: validate onread buffers before each read
an onread buffer can be detached after registration while native code still holds its address. validate the current view before each read and report an error instead of writing into the former backing store. Fixes: #67397 Signed-off-by: Fatih Güzel <fatihguzeldev@gmail.com>
1 parent 77e6379 commit 663b343

4 files changed

Lines changed: 334 additions & 7 deletions

File tree

‎src/stream_base.cc‎

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ namespace node {
1818

1919
using v8::Array;
2020
using v8::ArrayBuffer;
21+
using v8::ArrayBufferView;
2122
using v8::BackingStore;
2223
using v8::BackingStoreInitializationMode;
2324
using v8::ConstructorBehavior;
@@ -162,8 +163,8 @@ int StreamBase::ReadStopJS(const FunctionCallbackInfo<Value>& args) {
162163
int StreamBase::UseUserBuffer(const FunctionCallbackInfo<Value>& args) {
163164
CHECK(Buffer::HasInstance(args[0]));
164165

165-
uv_buf_t buf = uv_buf_init(Buffer::Data(args[0]), Buffer::Length(args[0]));
166-
PushStreamListener(new CustomBufferJSListener(buf));
166+
PushStreamListener(
167+
new CustomBufferJSListener(args.This(), args[0].As<ArrayBufferView>()));
167168
return 0;
168169
}
169170

@@ -765,9 +766,44 @@ void EmitToJSStreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
765766
stream->CallJSOnreadMethod(nread, ab, offset);
766767
}
767768

769+
CustomBufferJSListener::CustomBufferJSListener(Local<Object> object,
770+
Local<ArrayBufferView> view) {
771+
SetBuffer(object, view);
772+
}
773+
774+
void CustomBufferJSListener::SetBuffer(Local<Object> object,
775+
Local<ArrayBufferView> view) {
776+
// Keep the view's lifetime tied to the wrapper without a strong global root.
777+
object->SetInternalField(StreamBase::kUserBufferField, view);
778+
backing_store_ = view->Buffer()->GetBackingStore();
779+
}
768780

769781
uv_buf_t CustomBufferJSListener::OnStreamAlloc(size_t suggested_size) {
770-
return buffer_;
782+
CHECK_NOT_NULL(stream_);
783+
StreamBase* stream = static_cast<StreamBase*>(stream_);
784+
Environment* env = stream->stream_env();
785+
Isolate* isolate = env->isolate();
786+
HandleScope handle_scope(isolate);
787+
788+
Local<ArrayBufferView> view =
789+
stream->GetAsyncWrap()
790+
->object()
791+
->GetInternalField(StreamBase::kUserBufferField)
792+
.As<ArrayBufferView>();
793+
if (!backing_store_->IsImmutable()) {
794+
// SetBuffer() materializes the backing store, so this cannot copy.
795+
const auto contents = view->GetContents({});
796+
if (!contents.empty()) {
797+
buffer_ = uv_buf_init(reinterpret_cast<char*>(contents.data()),
798+
contents.size());
799+
return buffer_;
800+
}
801+
}
802+
803+
// Allocation must return a usable buffer. Report the invalid view when the
804+
// read completes, without writing into the user's former backing store.
805+
using_fallback_buffer_ = true;
806+
return env->allocate_managed_buffer(suggested_size);
771807
}
772808

773809

@@ -779,6 +815,13 @@ void CustomBufferJSListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
779815
HandleScope handle_scope(env->isolate());
780816
Context::Scope context_scope(env->context());
781817

818+
if (using_fallback_buffer_) {
819+
using_fallback_buffer_ = false;
820+
env->recycle_managed_buffer(env->release_managed_buffer(buf));
821+
stream->CallJSOnreadMethod(UV_EINVAL, Local<ArrayBuffer>());
822+
return;
823+
}
824+
782825
// In the case that there's an error and buf is null, return immediately.
783826
// This can happen on unices when POLLHUP is received and UV_EOF is returned
784827
// or when getting an error while performing a UV_HANDLE_ZERO_READ on Windows.
@@ -795,8 +838,9 @@ void CustomBufferJSListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
795838
StreamBase::SKIP_NREAD_CHECKS);
796839
Local<Value> next_buf_v;
797840
if (ret.ToLocal(&next_buf_v) && !next_buf_v->IsUndefined()) {
798-
buffer_.base = Buffer::Data(next_buf_v);
799-
buffer_.len = Buffer::Length(next_buf_v);
841+
CHECK(Buffer::HasInstance(next_buf_v));
842+
SetBuffer(stream->GetAsyncWrap()->object(),
843+
next_buf_v.As<ArrayBufferView>());
800844
}
801845
}
802846

‎src/stream_base.h‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "v8.h"
1212

1313
#include <climits> // INT_MIN
14+
#include <memory>
1415

1516
namespace node {
1617

@@ -211,10 +212,16 @@ class CustomBufferJSListener : public ReportWritesToJSStreamListener {
211212
void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override;
212213
void OnStreamDestroy() override { delete this; }
213214

214-
explicit CustomBufferJSListener(uv_buf_t buffer) : buffer_(buffer) {}
215+
CustomBufferJSListener(v8::Local<v8::Object> object,
216+
v8::Local<v8::ArrayBufferView> view);
215217

216218
private:
217-
uv_buf_t buffer_;
219+
void SetBuffer(v8::Local<v8::Object> object,
220+
v8::Local<v8::ArrayBufferView> view);
221+
222+
std::shared_ptr<v8::BackingStore> backing_store_;
223+
uv_buf_t buffer_ = uv_buf_init(nullptr, 0);
224+
bool using_fallback_buffer_ = false;
218225
};
219226

220227

@@ -314,6 +321,7 @@ class StreamBase : public StreamResource {
314321
kSlot = BaseObject::kSlot,
315322
kStreamBaseField = AsyncWrap::kInternalFieldCount,
316323
kOnReadFunctionField,
324+
kUserBufferField,
317325
kInternalFieldCount
318326
};
319327

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
// Flags: --js-immutable-arraybuffer
2+
'use strict';
3+
4+
// Reads must not modify an immutable ArrayBuffer created by transferring a
5+
// registered onread buffer.
6+
7+
const common = require('../common');
8+
const assert = require('node:assert');
9+
const { once } = require('node:events');
10+
const net = require('node:net');
11+
const { test } = require('node:test');
12+
13+
for (const mode of ['before first read', 'between reads', 'replacement buffer']) {
14+
test(`onread buffer transferred ${mode}`, async (t) => {
15+
const backing = new ArrayBuffer(8);
16+
new Uint8Array(backing).fill(42);
17+
const buffer = mode === 'replacement buffer' ?
18+
Buffer.from(backing, 2, 4) : new Uint8Array(backing, 2, 4);
19+
const firstBuffer = new Uint8Array(1);
20+
const connections = new Set();
21+
let peer;
22+
let socket;
23+
let immutable;
24+
let expected;
25+
let factoryCalls = 0;
26+
let readError;
27+
const closed = Promise.withResolvers();
28+
29+
const server = net.createServer(common.mustCall((connection) => {
30+
peer = connection;
31+
connection.on('error', (error) => {
32+
if (error.code !== 'ECONNRESET') throw error;
33+
});
34+
if (mode === 'before first read') {
35+
connection.end('test');
36+
} else {
37+
connection.write('a');
38+
}
39+
}));
40+
server.on('connection', (connection) => {
41+
connections.add(connection);
42+
connection.on('close', () => connections.delete(connection));
43+
});
44+
t.after(async () => {
45+
socket?.destroy();
46+
for (const connection of connections) connection.destroy();
47+
if (server.listening) {
48+
await new Promise((resolve) => server.close(resolve));
49+
}
50+
});
51+
server.listen(0, '127.0.0.1');
52+
await once(server, 'listening');
53+
54+
function transfer() {
55+
expected = [...new Uint8Array(backing)];
56+
immutable = backing.transferToImmutable();
57+
assert.strictEqual(buffer.byteLength, 0);
58+
assert.strictEqual(immutable.immutable, true);
59+
}
60+
61+
socket = net.connect({
62+
host: '127.0.0.1',
63+
port: server.address().port,
64+
onread: {
65+
buffer: mode === 'replacement buffer' ? common.mustCall(() => {
66+
return ++factoryCalls === 1 ? firstBuffer : buffer;
67+
}, 2) : buffer,
68+
callback: common.mustCall((nread, view) => {
69+
try {
70+
if (immutable !== undefined) {
71+
assert.deepStrictEqual([...new Uint8Array(immutable)], expected);
72+
assert.fail('onread called after the buffer was transferred');
73+
}
74+
assert.strictEqual(nread, 1);
75+
assert.strictEqual(view, mode === 'replacement buffer' ? firstBuffer : buffer);
76+
assert.strictEqual(view[0], 97);
77+
if (mode === 'replacement buffer') {
78+
// The factory's next view is registered after this callback.
79+
setImmediate(common.mustCall(() => {
80+
transfer();
81+
peer.end('test');
82+
}));
83+
} else {
84+
transfer();
85+
peer.end('test');
86+
}
87+
} catch (error) {
88+
closed.reject(error);
89+
return false;
90+
}
91+
}, mode === 'before first read' ? 0 : 1),
92+
},
93+
});
94+
socket.on('data', common.mustNotCall());
95+
socket.on('end', common.mustNotCall());
96+
socket.on('error', common.mustCall((error) => {
97+
readError = error;
98+
}));
99+
socket.on('close', common.mustCall(closed.resolve));
100+
if (mode === 'before first read') transfer();
101+
assert.strictEqual(await closed.promise, true);
102+
assert.strictEqual(readError.code, 'EINVAL');
103+
assert.strictEqual(readError.syscall, 'read');
104+
assert.deepStrictEqual([...new Uint8Array(immutable)], expected);
105+
});
106+
}
107+
108+
test('onread factory replaces buffers transferred after each read', async (t) => {
109+
const message = Buffer.from('test');
110+
const chunks = [];
111+
const closed = Promise.withResolvers();
112+
let peer;
113+
let socket;
114+
115+
const server = net.createServer(common.mustCall((connection) => {
116+
peer = connection;
117+
connection.on('error', (error) => {
118+
if (error.code !== 'ECONNRESET') throw error;
119+
});
120+
connection.end(message);
121+
}));
122+
t.after(async () => {
123+
socket?.destroy();
124+
peer?.destroy();
125+
if (server.listening) {
126+
await new Promise((resolve) => server.close(resolve));
127+
}
128+
});
129+
server.listen(0, '127.0.0.1');
130+
await once(server, 'listening');
131+
132+
socket = net.connect({
133+
host: '127.0.0.1',
134+
port: server.address().port,
135+
onread: {
136+
buffer: common.mustCall(() => new Uint8Array(1), message.length + 1),
137+
callback: common.mustCall((nread, view) => {
138+
try {
139+
assert.strictEqual(nread, 1);
140+
const immutable = view.buffer.transferToImmutable();
141+
assert.strictEqual(view.byteLength, 0);
142+
assert.strictEqual(immutable.immutable, true);
143+
chunks.push(immutable);
144+
} catch (error) {
145+
closed.reject(error);
146+
return false;
147+
}
148+
}, message.length),
149+
},
150+
});
151+
socket.on('data', common.mustNotCall());
152+
socket.on('end', common.mustCall());
153+
socket.on('error', closed.reject);
154+
socket.on('close', common.mustCall(closed.resolve));
155+
assert.strictEqual(await closed.promise, false);
156+
assert.deepStrictEqual(chunks.flatMap((chunk) => [...new Uint8Array(chunk)]), [...message]);
157+
});

0 commit comments

Comments
 (0)