message: "Assertion `val->IsArrayBufferView()' failed."

[a-shvedov]

Version

node-v16.13.2

Platform

Linux astra 4.15.3-141-generic #astra26+ci17 x86_64 GNU/Linux (debian-based)

Subsystem

No response

What steps will reproduce the bug?

\\1
var StringDecoder = require('string_decoder').StringDecoder;
var d = new StringDecoder('utf8');
var b = Buffer('abc');
console.log(b);
console.log(d.__proto__.write(b));

\\2
var StringDecoder = require('string_decoder').StringDecoder;
var d = new StringDecoder('utf8').__proto__;
var b = Buffer('abc');
console.log(b);
console.log(d.write(b));

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

Standart buildCompiled with: gcc (version 8.3.0) ;
Configure params: --prefix="/target/dir";
Builded binary info:
node: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, not stripped.

What is the expected behavior?

No response

What do you see instead?

Stderr with run crafted sample: Abort;

<Buffer 61 62 63>
./node[7121]: ../src/node_buffer.cc:246:char *node::Buffer::Data(Local<v8::Value>): Assertion `val->IsArrayBufferView()' failed.
 1: 0x10b84c7 node::DumpBacktrace(_IO_FILE*) [./node]
 2: 0x17584a1 node::Abort() [./node]
 3: 0x1756c1e  [./node]
 4: 0x15509af node::Buffer::Data(v8::Local<v8::Value>) [./node]
 5: 0x213f228  [./node]
 6: 0x2a7f008 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [./node]
 7: 0x2a7e532  [./node]
 8: 0x2a7d1ca v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [./node]
 9: 0x3cb9299  [./node]
Аварийный останов

Additional information

Call might be what is causing in type proto (the last dot).

[benjamingr]

I'm not sure I understand what the ask here is - to fail with a catchable exception?

Something like:

diff --git a/lib/string_decoder.js b/lib/string_decoder.js
index 7447bb3f46..ab6bfb306b 100644
--- a/lib/string_decoder.js
+++ b/lib/string_decoder.js
@@ -42,6 +42,7 @@ const {
 } = internalBinding('string_decoder');
 const internalUtil = require('internal/util');
 const {
+  ERR_ILLEGAL_CONSTRUCTOR,
   ERR_INVALID_ARG_TYPE,
   ERR_UNKNOWN_ENCODING
 } = require('internal/errors').codes;
@@ -101,6 +102,9 @@ StringDecoder.prototype.write = function write(buf) {
     throw new ERR_INVALID_ARG_TYPE('buf',
                                    ['Buffer', 'TypedArray', 'DataView'],
                                    buf);
+  if (!this[kNativeDecoder]) {
+    throw new ERR_ILLEGAL_CONSTRUCTOR();
+  }
   return decode(this[kNativeDecoder], buf);
 };

[benjamingr]

My intuition is "we shouldn't guard against this sort of thing" most likely. @addaleax wdyt?

[addaleax]

I mean, I personally think that it’s fine to add guards (in C++, not JS, to keep performance parity), but I think that ship has sailed for Node.js internals.

[RaisinTen]

C++ fix - #42061
JS fix - #42062
Running benchmarks on both to be sure about the differences.

[a-shvedov]

Great, I think this issue can be closed.

[RaisinTen]

We can keep this issue open for now and it will get closed automatically when any of the PRs land since I have added Fixes: <issue-link> to the descriptions. :)

[a-shvedov]

OK Thanks!

[RaisinTen]

@addaleax there doesn't appear to be any significant perf regressions in the JS fix or the C++ fix, so should we go ahead with the JS fix, given that we mostly do such validations in JS?