string_decoder: fix crash when calling __proto__.write()

This makes the function throw an exception from C++ instead of crashing. Fixes: nodejs#41949 Signed-off-by: Darshan Sen <[email protected]>
RaisinTen · Feb 20, 2022 · c138a51 · c138a51
1 parent dde2f78
commit c138a51
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 6 deletions.
diff --git a/src/node_errors.h b/src/node_errors.h
@@ -35,6 +35,7 @@ void OnFatalError(const char* location, const char* message);
   V(ERR_CLOSED_MESSAGE_PORT, Error)                                            \
   V(ERR_CONSTRUCT_CALL_REQUIRED, TypeError)                                    \
   V(ERR_CONSTRUCT_CALL_INVALID, TypeError)                                     \
+  V(ERR_ILLEGAL_CONSTRUCTOR, TypeError)                                        \
   V(ERR_CRYPTO_INITIALIZATION_FAILED, Error)                                   \
   V(ERR_CRYPTO_INVALID_AUTH_TAG, TypeError)                                    \
   V(ERR_CRYPTO_INVALID_COUNTER, TypeError)                                     \
@@ -124,6 +125,7 @@ ERRORS_WITH_CODE(V)
     "Buffer is not available for the current Context")                         \
   V(ERR_CLOSED_MESSAGE_PORT, "Cannot send data on closed MessagePort")         \
   V(ERR_CONSTRUCT_CALL_INVALID, "Constructor cannot be called")                \
+  V(ERR_ILLEGAL_CONSTRUCTOR, "Illegal constructor")                            \
   V(ERR_CONSTRUCT_CALL_REQUIRED, "Cannot call constructor without `new`")      \
   V(ERR_CRYPTO_INITIALIZATION_FAILED, "Initialization failed")                 \
   V(ERR_CRYPTO_INVALID_AUTH_TAG, "Invalid authentication tag")                 \

diff --git a/src/string_decoder.cc b/src/string_decoder.cc
@@ -37,7 +37,7 @@ MaybeLocal<String> MakeString(Isolate* isolate,
         v8::NewStringType::kNormal,
         length);
     if (utf8_string.IsEmpty()) {
-      isolate->ThrowException(node::ERR_STRING_TOO_LONG(isolate));
+      isolate->ThrowException(ERR_STRING_TOO_LONG(isolate));
       return MaybeLocal<String>();
     } else {
       return utf8_string;
@@ -264,18 +264,28 @@ MaybeLocal<String> StringDecoder::FlushData(Isolate* isolate) {
 namespace {
 
 void DecodeData(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+
+  Local<Value> decoder_value = args[0];
+  if (!decoder_value->IsArrayBufferView()) {
+    isolate->ThrowException(ERR_ILLEGAL_CONSTRUCTOR(isolate));
+    return;
+  }
+
   StringDecoder* decoder =
-      reinterpret_cast<StringDecoder*>(Buffer::Data(args[0]));
+      reinterpret_cast<StringDecoder*>(Buffer::Data(decoder_value));
   CHECK_NOT_NULL(decoder);
 
   CHECK(args[1]->IsArrayBufferView());
   ArrayBufferViewContents<char> content(args[1].As<ArrayBufferView>());
   size_t length = content.length();
 
-  MaybeLocal<String> ret =
-      decoder->DecodeData(args.GetIsolate(), content.data(), &length);
-  if (!ret.IsEmpty())
-    args.GetReturnValue().Set(ret.ToLocalChecked());
+  Local<String> decoded_data;
+  if (!decoder->DecodeData(isolate,
+                           content.data(),
+                           &length).ToLocal(&decoded_data)) return;
+
+  args.GetReturnValue().Set(decoded_data);
 }
 
 void FlushData(const FunctionCallbackInfo<Value>& args) {

diff --git a/test/parallel/test-string-decoder.js b/test/parallel/test-string-decoder.js
@@ -210,6 +210,13 @@ if (common.enoughTestMem) {
   );
 }
 
+assert.throws(
+  () => new StringDecoder('utf8').__proto__.write(Buffer.from('abc')), // eslint-disable-line no-proto
+  {
+    code: 'ERR_ILLEGAL_CONSTRUCTOR',
+  }
+);
+
 // Test verifies that StringDecoder will correctly decode the given input
 // buffer with the given encoding to the expected output. It will attempt all
 // possible ways to write() the input buffer, see writeSequences(). The