StringDecoder undocumented size limit

[TanninOne]

• Version: v12.13.0
• Platform: Windows 10 (64bit)
• Subsystem: string_decoder, stream

What steps will reproduce the bug?

const buf = Buffer.alloc(1024 * 1024 * 1024, 'a');
const sd = new (require('string_decoder').StringDecoder)();
sd.write(buf).length

How often does it reproduce? Is there a required condition?

The above causes the issue 100% reliably for me.

What is the expected behavior?

Either the decoded buffer or an exception explaining that the input buffer is too long.

What do you see instead?

TypeError: Cannot read property 'length' of undefined

So sd.write(large buffer) returns undefined

The reason I'm including stream as an affected subsystem is this line which originally led me to this issue: https://github.com/nodejs/readable-stream/blob/040b813e60ecf0d68ac1461a3fc3157ea5785950/lib/_stream_readable.js#L277

So what happens is that if you have a stream (a pipe used for IPC in my case) and the other side sends an large chunk of data, the recipient throws an exception before any application code is invoked that could handle it gracefully.

Additional information

I don't know exactly how large the buffer has to be. In my application (electron 8.5.2 with node 12.13.0) this started happening at around 600MB, when reproducing in node on the command line I had to increase the buffer to 1GB.

Just to clarify: I understand that there will be a limit on how large these buffers can get, I just wish it would report an error that my application code can handle because right now I don't see how I could write robust client code that can deal with the other side sending crap.

[targos]

The size limit is exactly 0x1fffffe8 (536870888) characters. That's the maximum length a JS string can have in the V8 engine. The method correctly throws an error if the decoder is setup with ascii encoding. I'm looking into why that doesn't happen with utf-8

[targos]

Well, I don't know why there's no exception propagating to JS...

Relevant lines in the code:

node/src/string_decoder.cc

Lines 265 to 268 in d146b25

	MaybeLocal<String> ret =
	decoder->DecodeData(args.GetIsolate(), content.data(), &length);
	if (!ret.IsEmpty())
	args.GetReturnValue().Set(ret.ToLocalChecked());

node/src/string_decoder.cc

Lines 208 to 211 in d146b25

	if (nread > 0) {
	if (!MakeString(isolate, data, nread, Encoding()).ToLocal(&body))
	return MaybeLocal<String>();
	} else {

node/src/string_decoder.cc

Lines 32 to 38 in d146b25

	if (encoding == UTF8) {
	return String::NewFromUtf8(
	isolate,
	data,
	v8::NewStringType::kNormal,
	length);
	} else {

/cc @addaleax

[targos]

It's like String::NewFromUtf8 returns an empty MaybeLocal<String> but without a pending exception.

[addaleax]

It's like String::NewFromUtf8 returns an empty MaybeLocal<String> but without a pending exception.

Yeah, it does – that’s a long-standing TODO in the V8 source… we should probably throw one ourselves :/

[targos]

I see... Working on it!

[targos]

PR at #36661