The Wayback Machine - https://web.archive.org/web/20250407172852/https://github.com/nodejs/node/commit/a9d528be5b
Skip to content

Navigation Menu

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

Commit a9d528b

Browse filesBrowse files
trevnorrisMylesBorins
authored andcommitted
buffer: fix range checks for slice()
Using the black magic of Symbol.toPrimitive the numeric value of start/end can be changed when Uint32Value() is called once Buffer::Fill() is entered. Allowing the CHECK() to be bypassed. The bug report was only for "start", but the same can be done with "end". Perform checks for both in node::Buffer::Fill() to make sure the issue can't be triggered, even if process.binding is used directly. Include tests for each case. Along with a check to make sure the last time the value is accessed returns -1. This should be enough to make sure Buffer::Fill() is receiving the correct value. Along with two tests against process.binding directly. Fixes: #9149 PR-URL: #9174 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <ranziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
1 parent fa7d378 commit a9d528b
Copy full SHA for a9d528b

File tree

2 files changed

+78
-1
lines changed

2 files changed

+78
-1
lines changed

‎src/node_buffer.cc

Copy file name to clipboardexpand all lines: src/node_buffer.cc
+2-1
Original file line numberDiff line numberDiff line change
@@ -580,7 +580,8 @@ void Fill(const FunctionCallbackInfo<Value>& args) {
580580
Local<String> str_obj;
581581
size_t str_length;
582582
enum encoding enc;
583-
CHECK(fill_length + start <= ts_obj_length);
583+
THROW_AND_RETURN_IF_OOB(start <= end);
584+
THROW_AND_RETURN_IF_OOB(fill_length + start <= ts_obj_length);
584585

585586
// First check if Buffer has been passed.
586587
if (Buffer::HasInstance(args[1])) {

‎test/parallel/test-buffer-fill.js

Copy file name to clipboardexpand all lines: test/parallel/test-buffer-fill.js
+76
Original file line numberDiff line numberDiff line change
@@ -314,3 +314,79 @@ Buffer.alloc(8, '');
314314
buf.fill('է');
315315
assert.strictEqual(buf.toString(), 'էէէէէ');
316316
}
317+
318+
// Testing public API. Make sure "start" is properly checked, even if it's
319+
// magically mangled using Symbol.toPrimitive.
320+
{
321+
let elseWasLast = false;
322+
assert.throws(() => {
323+
var ctr = 0;
324+
const start = {
325+
[Symbol.toPrimitive]() {
326+
// We use this condition to get around the check in lib/buffer.js
327+
if (ctr <= 0) {
328+
elseWasLast = false;
329+
ctr = ctr + 1;
330+
return 0;
331+
} else {
332+
elseWasLast = true;
333+
// Once buffer.js calls the C++ implemenation of fill, return -1
334+
return -1;
335+
}
336+
}
337+
};
338+
Buffer.alloc(1).fill(Buffer.alloc(1), start, 1);
339+
}, /out of range index/);
340+
// Make sure -1 is making it to Buffer::Fill().
341+
assert.ok(elseWasLast,
342+
'internal API changed, -1 no longer in correct location');
343+
}
344+
345+
// Testing process.binding. Make sure "start" is properly checked for -1 wrap
346+
// around.
347+
assert.throws(() => {
348+
process.binding('buffer').fill(Buffer.alloc(1), 1, -1, 0, 1);
349+
}, /out of range index/);
350+
351+
// Make sure "end" is properly checked, even if it's magically mangled using
352+
// Symbol.toPrimitive.
353+
{
354+
let elseWasLast = false;
355+
assert.throws(() => {
356+
var ctr = 0;
357+
const end = {
358+
[Symbol.toPrimitive]() {
359+
// We use this condition to get around the check in lib/buffer.js
360+
if (ctr <= 1) {
361+
elseWasLast = false;
362+
ctr = ctr + 1;
363+
return 1;
364+
} else {
365+
elseWasLast = true;
366+
// Once buffer.js calls the C++ implemenation of fill, return -1
367+
return -1;
368+
}
369+
}
370+
};
371+
Buffer.alloc(1).fill(Buffer.alloc(1), 0, end);
372+
});
373+
// Make sure -1 is making it to Buffer::Fill().
374+
assert.ok(elseWasLast,
375+
'internal API changed, -1 no longer in correct location');
376+
}
377+
378+
// Testing process.binding. Make sure "end" is properly checked for -1 wrap
379+
// around.
380+
assert.throws(() => {
381+
process.binding('buffer').fill(Buffer.alloc(1), 1, 1, -2, 1);
382+
}, /out of range index/);
383+
384+
// Test that bypassing 'length' won't cause an abort.
385+
assert.throws(() => {
386+
const buf = new Buffer('w00t');
387+
Object.defineProperty(buf, 'length', {
388+
value: 1337,
389+
enumerable: true
390+
});
391+
buf.fill('');
392+
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.