From 63eb267c34198c4bece62cb6432bb2bceb399de6 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 2 Apr 2018 10:39:26 +0800 Subject: [PATCH] src: migrate string_bytes.cc to throw errors with code PR-URL: https://github.com/nodejs/node/pull/19739 Fixes: https://github.com/nodejs/node/issues/3175 Fixes: https://github.com/nodejs/node/issues/9489 Reviewed-By: Gus Caplan Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- src/string_bytes.cc | 41 ++++++++----------- ...ingbytes-external-exceed-max-by-1-ascii.js | 10 +++-- ...ngbytes-external-exceed-max-by-1-base64.js | 10 +++-- ...ngbytes-external-exceed-max-by-1-binary.js | 9 +++- ...tringbytes-external-exceed-max-by-1-hex.js | 10 +++-- ...ringbytes-external-exceed-max-by-1-utf8.js | 23 +++++++++-- .../test-stringbytes-external-exceed-max.js | 11 +++-- .../test-fs-readfile-tostring-fail.js | 11 ++++- 8 files changed, 81 insertions(+), 44 deletions(-) diff --git a/src/string_bytes.cc b/src/string_bytes.cc index 624eb92b930c12..bbd381d33a5cf7 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -23,6 +23,7 @@ #include "base64.h" #include "node_internals.h" +#include "node_errors.h" #include "node_buffer.h" #include @@ -36,20 +37,6 @@ // use external string resources. #define EXTERN_APEX 0xFBEE9 -// TODO(addaleax): These should all have better error messages. In particular, -// they should mention what the actual limits are. -#define SB_MALLOC_FAILED_ERROR \ - v8::Exception::Error(OneByteString(isolate, "\"toString()\" failed")) - -#define SB_STRING_TOO_LONG_ERROR \ - v8::Exception::Error(OneByteString(isolate, "\"toString()\" failed")) - -#define SB_BUFFER_CREATION_ERROR \ - v8::Exception::Error(OneByteString(isolate, "\"toString()\" failed")) - -#define SB_BUFFER_SIZE_EXCEEDED_ERROR \ - v8::Exception::Error(OneByteString(isolate, "\"toString()\" failed")) - namespace node { using v8::HandleScope; @@ -93,7 +80,7 @@ class ExternString: public ResourceType { TypeName* new_data = node::UncheckedMalloc(length); if (new_data == nullptr) { - *error = SB_MALLOC_FAILED_ERROR; + *error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate); return MaybeLocal(); } memcpy(new_data, data, length * sizeof(*new_data)); @@ -126,7 +113,7 @@ class ExternString: public ResourceType { if (str.IsEmpty()) { delete h_str; - *error = SB_STRING_TOO_LONG_ERROR; + *error = node::ERR_STRING_TOO_LARGE(isolate); return MaybeLocal(); } @@ -183,7 +170,7 @@ MaybeLocal ExternOneByteString::NewSimpleFromCopy(Isolate* isolate, v8::NewStringType::kNormal, length); if (str.IsEmpty()) { - *error = SB_STRING_TOO_LONG_ERROR; + *error = node::ERR_STRING_TOO_LARGE(isolate); return MaybeLocal(); } return str.ToLocalChecked(); @@ -201,7 +188,7 @@ MaybeLocal ExternTwoByteString::NewSimpleFromCopy(Isolate* isolate, v8::NewStringType::kNormal, length); if (str.IsEmpty()) { - *error = SB_STRING_TOO_LONG_ERROR; + *error = node::ERR_STRING_TOO_LARGE(isolate); return MaybeLocal(); } return str.ToLocalChecked(); @@ -616,7 +603,7 @@ static size_t hex_encode(const char* src, size_t slen, char* dst, size_t dlen) { #define CHECK_BUFLEN_IN_RANGE(len) \ do { \ if ((len) > Buffer::kMaxLength) { \ - *error = SB_BUFFER_SIZE_EXCEEDED_ERROR; \ + *error = node::ERR_BUFFER_TOO_LARGE(isolate); \ return MaybeLocal(); \ } \ } while (0) @@ -639,9 +626,13 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, switch (encoding) { case BUFFER: { + if (buflen > node::Buffer::kMaxLength) { + *error = node::ERR_BUFFER_TOO_LARGE(isolate); + return MaybeLocal(); + } auto maybe_buf = Buffer::Copy(isolate, buf, buflen); if (maybe_buf.IsEmpty()) { - *error = SB_BUFFER_CREATION_ERROR; + *error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate); return MaybeLocal(); } return maybe_buf.ToLocalChecked(); @@ -651,7 +642,7 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, if (contains_non_ascii(buf, buflen)) { char* out = node::UncheckedMalloc(buflen); if (out == nullptr) { - *error = SB_MALLOC_FAILED_ERROR; + *error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate); return MaybeLocal(); } force_ascii(buf, out, buflen); @@ -666,7 +657,7 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, v8::NewStringType::kNormal, buflen); if (val.IsEmpty()) { - *error = SB_STRING_TOO_LONG_ERROR; + *error = node::ERR_STRING_TOO_LARGE(isolate); return MaybeLocal(); } return val.ToLocalChecked(); @@ -678,7 +669,7 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, size_t dlen = base64_encoded_size(buflen); char* dst = node::UncheckedMalloc(dlen); if (dst == nullptr) { - *error = SB_MALLOC_FAILED_ERROR; + *error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate); return MaybeLocal(); } @@ -692,7 +683,7 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, size_t dlen = buflen * 2; char* dst = node::UncheckedMalloc(dlen); if (dst == nullptr) { - *error = SB_MALLOC_FAILED_ERROR; + *error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate); return MaybeLocal(); } size_t written = hex_encode(buf, buflen, dst, dlen); @@ -723,7 +714,7 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, if (IsBigEndian()) { uint16_t* dst = node::UncheckedMalloc(buflen); if (dst == nullptr) { - *error = SB_MALLOC_FAILED_ERROR; + *error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate); return MaybeLocal(); } size_t nbytes = buflen * sizeof(uint16_t); diff --git a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii.js b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii.js index 43b8c63f1e466e..96a3273254d8d7 100644 --- a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii.js +++ b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii.js @@ -6,7 +6,6 @@ if (!common.enoughTestMem) common.skip(skipMessage); const binding = require(`./build/${common.buildType}/binding`); -const assert = require('assert'); // v8 fails silently if string length > v8::String::kMaxLength // v8::String::kMaxLength defined in v8.h @@ -25,6 +24,11 @@ try { if (!binding.ensureAllocation(2 * kStringMaxLength)) common.skip(skipMessage); -assert.throws(function() { +const stringLengthHex = kStringMaxLength.toString(16); +common.expectsError(function() { buf.toString('ascii'); -}, /"toString\(\)" failed/); +}, { + message: `Cannot create a string larger than 0x${stringLengthHex} bytes`, + code: 'ERR_STRING_TOO_LARGE', + type: Error +}); diff --git a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64.js b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64.js index a94a57e18037fd..90e13ce788efcc 100644 --- a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64.js +++ b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64.js @@ -6,7 +6,6 @@ if (!common.enoughTestMem) common.skip(skipMessage); const binding = require(`./build/${common.buildType}/binding`); -const assert = require('assert'); // v8 fails silently if string length > v8::String::kMaxLength // v8::String::kMaxLength defined in v8.h @@ -25,6 +24,11 @@ try { if (!binding.ensureAllocation(2 * kStringMaxLength)) common.skip(skipMessage); -assert.throws(function() { +const stringLengthHex = kStringMaxLength.toString(16); +common.expectsError(function() { buf.toString('base64'); -}, /"toString\(\)" failed/); +}, { + message: `Cannot create a string larger than 0x${stringLengthHex} bytes`, + code: 'ERR_STRING_TOO_LARGE', + type: Error +}); diff --git a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary.js b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary.js index 996c01752da7c6..0ffd1eb4166989 100644 --- a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary.js +++ b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary.js @@ -25,9 +25,14 @@ try { if (!binding.ensureAllocation(2 * kStringMaxLength)) common.skip(skipMessage); -assert.throws(function() { +const stringLengthHex = kStringMaxLength.toString(16); +common.expectsError(function() { buf.toString('latin1'); -}, /"toString\(\)" failed/); +}, { + message: `Cannot create a string larger than 0x${stringLengthHex} bytes`, + code: 'ERR_STRING_TOO_LARGE', + type: Error +}); let maxString = buf.toString('latin1', 1); assert.strictEqual(maxString.length, kStringMaxLength); diff --git a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex.js b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex.js index 17d9ae5d68f033..bc64ef396dc2d3 100644 --- a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex.js +++ b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex.js @@ -6,7 +6,6 @@ if (!common.enoughTestMem) common.skip(skipMessage); const binding = require(`./build/${common.buildType}/binding`); -const assert = require('assert'); // v8 fails silently if string length > v8::String::kMaxLength // v8::String::kMaxLength defined in v8.h @@ -25,6 +24,11 @@ try { if (!binding.ensureAllocation(2 * kStringMaxLength)) common.skip(skipMessage); -assert.throws(function() { +const stringLengthHex = kStringMaxLength.toString(16); +common.expectsError(function() { buf.toString('hex'); -}, /"toString\(\)" failed/); +}, { + message: `Cannot create a string larger than 0x${stringLengthHex} bytes`, + code: 'ERR_STRING_TOO_LARGE', + type: Error +}); diff --git a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8.js b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8.js index d3368ca7b2ea6b..f6c9f21e4b06e2 100644 --- a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8.js +++ b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8.js @@ -25,10 +25,27 @@ try { if (!binding.ensureAllocation(2 * kStringMaxLength)) common.skip(skipMessage); +const stringLengthHex = kStringMaxLength.toString(16); + assert.throws(function() { buf.toString(); -}, /"toString\(\)" failed|Array buffer allocation failed/); +}, function(e) { + if (e.message !== 'Array buffer allocation failed') { + common.expectsError({ + message: `Cannot create a string larger than 0x${stringLengthHex} bytes`, + code: 'ERR_STRING_TOO_LARGE', + type: Error + })(e); + return true; + } else { + return true; + } +}); -assert.throws(function() { +common.expectsError(function() { buf.toString('utf8'); -}, /"toString\(\)" failed/); +}, { + message: `Cannot create a string larger than 0x${stringLengthHex} bytes`, + code: 'ERR_STRING_TOO_LARGE', + type: Error +}); diff --git a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js index 319a8a93558c45..e4b66d8f30bae3 100644 --- a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js +++ b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js @@ -6,7 +6,6 @@ if (!common.enoughTestMem) common.skip(skipMessage); const binding = require(`./build/${common.buildType}/binding`); -const assert = require('assert'); // v8 fails silently if string length > v8::String::kMaxLength // v8::String::kMaxLength defined in v8.h @@ -25,6 +24,12 @@ try { if (!binding.ensureAllocation(2 * kStringMaxLength)) common.skip(skipMessage); -assert.throws(function() { +const stringLengthHex = kStringMaxLength.toString(16); + +common.expectsError(function() { buf.toString('utf16le'); -}, /"toString\(\)" failed/); +}, { + message: `Cannot create a string larger than 0x${stringLengthHex} bytes`, + code: 'ERR_STRING_TOO_LARGE', + type: Error +}); diff --git a/test/sequential/test-fs-readfile-tostring-fail.js b/test/sequential/test-fs-readfile-tostring-fail.js index 81c7a941eb9476..f8b0c666a01b23 100644 --- a/test/sequential/test-fs-readfile-tostring-fail.js +++ b/test/sequential/test-fs-readfile-tostring-fail.js @@ -32,8 +32,15 @@ stream.end(); stream.on('finish', common.mustCall(function() { fs.readFile(file, 'utf8', common.mustCall(function(err, buf) { assert.ok(err instanceof Error); - assert.ok(/^(Array buffer allocation failed|"toString\(\)" failed)$/ - .test(err.message)); + if (err.message !== 'Array buffer allocation failed') { + const stringLengthHex = kStringMaxLength.toString(16); + common.expectsError({ + message: 'Cannot create a string larger than ' + + `0x${stringLengthHex} bytes`, + code: 'ERR_STRING_TOO_LARGE', + type: Error + })(err); + } assert.strictEqual(buf, undefined); })); }));