Skip to content

stream: disallow writing string chunk with 'buffer' encoding #63062

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

Open
Renegade334 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

stream: disallow writing string chunk with 'buffer' encoding #63062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions lib/internal/streams/writable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,9 @@ function _write(stream, chunk, encoding, cb) {
}

if (typeof chunk === 'string') {
if (encoding === 'buffer') {
throw new ERR_UNKNOWN_ENCODING(encoding);
}
if ((state[kState] & kDecodeStrings) !== 0) {
chunk = Buffer.from(chunk, encoding);
encoding = 'buffer';
Expand Down
18 changes: 0 additions & 18 deletions test/parallel/test-stream-base-typechecking.js
View file
Open in desktop
Copy link
Copy Markdown
Member Author

@Renegade334 Renegade334 May 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This was a test for the behaviour reported in #12152, where calling .write('string', 'buffer') on a StreamBase-backed stream would crash the process. I don't believe there's any way to hit this if this change lands, but not confidently enough to revert back to a CHECK (cf. #12753) without a second opinion – maybe Team Stream has an insight?

(cc @addaleax)

This file was deleted.

22 changes: 15 additions & 7 deletions test/parallel/test-stream-writable-write-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function test(autoDestroy) {
{
const w = new Writable({
autoDestroy,
_write() {}
write() {}
Copy link
Copy Markdown
Member Author

@Renegade334 Renegade334 May 1, 2026

Choose a reason for hiding this comment

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

These tests were using the wrong constructor option name for setting the _write implementation. It doesn't matter if the error tests fail as expected, but it does make the assertion message less clear if an error test doesn't fail, as it instead complains about a missing _write implementation rather than reporting the missing expected error.

});
w.end();
expectError(w, ['asd'], 'ERR_STREAM_WRITE_AFTER_END');
Expand All @@ -40,34 +40,42 @@ function test(autoDestroy) {
{
const w = new Writable({
autoDestroy,
_write() {}
write() {}
});
w.destroy();
}

{
const w = new Writable({
autoDestroy,
_write() {}
write() {}
});
expectError(w, [null], 'ERR_STREAM_NULL_VALUES', true);
}

{
const w = new Writable({
autoDestroy,
_write() {}
write() {}
});
expectError(w, [{}], 'ERR_INVALID_ARG_TYPE', true);
}

{
const w = new Writable({
decodeStrings: false,
autoDestroy,
_write() {}
write() {}
});
expectError(w, ['asd', 'buffer'], 'ERR_UNKNOWN_ENCODING', true);
}

{
const w = new Writable({
autoDestroy,
decodeStrings: false,
write() {}
});
expectError(w, ['asd', 'noencoding'], 'ERR_UNKNOWN_ENCODING', true);
expectError(w, ['asd', 'buffer'], 'ERR_UNKNOWN_ENCODING', true);
}
}

Expand Down
Loading