Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

buffer: fix single-character string filling #9837

Closed
wants to merge 2 commits into from
Closed

buffer: fix single-character string filling #9837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cea2194
buffer: handle UCS2 `.fill()` properly on BE
addaleax Dec 6, 2016
b0a80b7
buffer: fix single-character string filling
addaleax Nov 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,23 +672,27 @@ Buffer.prototype.fill = function fill(val, start, end, encoding) {
encoding = end;
end = this.length;
}
if (val.length === 1) {
var code = val.charCodeAt(0);
if (code < 256)
val = code;
}
if (val.length === 0) {
// Previously, if val === '', the Buffer would not fill,
// which is rather surprising.
val = 0;
}

if (encoding !== undefined && typeof encoding !== 'string') {
throw new TypeError('encoding must be a string');
}
if (typeof encoding === 'string' && !Buffer.isEncoding(encoding)) {
var normalizedEncoding = internalUtil.normalizeEncoding(encoding);
if (normalizedEncoding === undefined) {
throw new TypeError('Unknown encoding: ' + encoding);
}

if (val.length === 0) {
// Previously, if val === '', the Buffer would not fill,
// which is rather surprising.
val = 0;
} else if (val.length === 1) {
var code = val.charCodeAt(0);
if ((normalizedEncoding === 'utf8' && code < 128) ||
normalizedEncoding === 'latin1') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backwards compatibility do we still need to check for 'binary', or is that no longer supported? If this is back ported then it'll probably need to be checked there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris I switched the blocks around here so that this is after normalizeEncoding, so there’s no need to worry about alternate names

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool. thanks.

// Fast path: If `val` fits into a single byte, use that numeric value.
val = code;
}
}
} else if (typeof val === 'number') {
val = val & 255;
}
Expand Down
3 changes: 3 additions & 0 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,9 @@ void Fill(const FunctionCallbackInfo<Value>& args) {

} else if (enc == UCS2) {
node::TwoByteValue str(env->isolate(), args[1]);
if (IsBigEndian())
SwapBytes16(reinterpret_cast<char*>(&str[0]), str_length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your comment sounds like just this needs to be removed right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have made this clearer, but this is already the working version; i.e. this line was missing and should already have been there afaict


memcpy(ts_obj_data + start, *str, MIN(str_length, fill_length));

} else {
Expand Down
49 changes: 36 additions & 13 deletions test/parallel/test-buffer-fill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@

require('../common');
const assert = require('assert');
const os = require('os');
const SIZE = 28;

const buf1 = Buffer.allocUnsafe(SIZE);
const buf2 = Buffer.allocUnsafe(SIZE);


// Default encoding
testBufs('abc');
testBufs('\u0222aa');
Expand Down Expand Up @@ -112,8 +110,7 @@ testBufs('\u0222aa', 8, 1, 'ucs2');
testBufs('a\u0234b\u0235c\u0236', 4, -1, 'ucs2');
testBufs('a\u0234b\u0235c\u0236', 4, 1, 'ucs2');
testBufs('a\u0234b\u0235c\u0236', 12, 1, 'ucs2');
assert.strictEqual(Buffer.allocUnsafe(1).fill('\u0222', 'ucs2')[0],
os.endianness() === 'LE' ? 0x22 : 0x02);
assert.strictEqual(Buffer.allocUnsafe(1).fill('\u0222', 'ucs2')[0], 0x22);


// HEX
Expand Down Expand Up @@ -259,15 +256,6 @@ function writeToFill(string, offset, end, encoding) {
}
} while (offset < buf2.length);

// Correction for UCS2 operations.
if (os.endianness() === 'BE' && encoding === 'ucs2') {
for (var i = 0; i < buf2.length; i += 2) {
var tmp = buf2[i];
buf2[i] = buf2[i + 1];
buf2[i + 1] = tmp;
}
}

return buf2;
}

Expand Down Expand Up @@ -406,3 +394,38 @@ assert.throws(() => {
});
buf.fill('');
}, /^RangeError: out of range index$/);

assert.deepStrictEqual(
Buffer.allocUnsafeSlow(16).fill('ab', 'utf16le'),
Buffer.from('61006200610062006100620061006200', 'hex'));

assert.deepStrictEqual(
Buffer.allocUnsafeSlow(15).fill('ab', 'utf16le'),
Buffer.from('610062006100620061006200610062', 'hex'));

assert.deepStrictEqual(
Buffer.allocUnsafeSlow(16).fill('ab', 'utf16le'),
Buffer.from('61006200610062006100620061006200', 'hex'));
assert.deepStrictEqual(
Buffer.allocUnsafeSlow(16).fill('a', 'utf16le'),
Buffer.from('61006100610061006100610061006100', 'hex'));

assert.strictEqual(
Buffer.allocUnsafeSlow(16).fill('a', 'utf16le').toString('utf16le'),
'a'.repeat(8));
assert.strictEqual(
Buffer.allocUnsafeSlow(16).fill('a', 'latin1').toString('latin1'),
'a'.repeat(16));
assert.strictEqual(
Buffer.allocUnsafeSlow(16).fill('a', 'utf8').toString('utf8'),
'a'.repeat(16));

assert.strictEqual(
Buffer.allocUnsafeSlow(16).fill('Љ', 'utf16le').toString('utf16le'),
'Љ'.repeat(8));
assert.strictEqual(
Buffer.allocUnsafeSlow(16).fill('Љ', 'latin1').toString('latin1'),
'\t'.repeat(16));
assert.strictEqual(
Buffer.allocUnsafeSlow(16).fill('Љ', 'utf8').toString('utf8'),
'Љ'.repeat(8));