From cde7fde35c254c29bb99fae9ba22a6e6b146b3d7 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 9 Nov 2019 16:14:24 +0300 Subject: [PATCH 1/2] util: fix .format() not always calling toString when it should be This makes sure that `util.format('%s', object)` will always call a user defined `toString` function. It was formerly not the case when the object had the function declared on the super class. At the same time this also makes sure that getters won't be triggered accessing the `constructor` property. --- lib/internal/util/inspect.js | 57 +++++++++++++++++------------ test/parallel/test-util-format.js | 60 +++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 22 deletions(-) diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index 3e93a41795c8d8..ec4e02751a6ce3 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -1571,6 +1571,31 @@ function reduceToSingleString( return `${braces[0]}${ln}${join(output, `,\n${indentation} `)} ${braces[1]}`; } +function hasBuiltInToString(value) { + // Count objects that have no `toString` function as built-in. + if (typeof value.toString !== 'function') { + return true; + } + + // The object has a own `toString` property. Thus it's not not a built-in one. + if (hasOwnProperty(value, 'toString')) { + return false; + } + + // Find the object that has the `toString` property as own property in the + // prototype chain. + let pointer = value; + do { + pointer = ObjectGetPrototypeOf(pointer); + } while (!hasOwnProperty(pointer, 'toString')); + + // Check closer if the object is a built-in. + const descriptor = ObjectGetOwnPropertyDescriptor(pointer, 'constructor'); + return descriptor !== undefined && + typeof descriptor.value === 'function' && + builtInObjects.has(descriptor.value.name); +} + const firstErrorLine = (error) => error.message.split('\n')[0]; let CIRCULAR_ERROR_MESSAGE; function tryStringify(arg) { @@ -1629,29 +1654,17 @@ function formatWithOptionsInternal(inspectOptions, ...args) { tempStr = formatNumber(stylizeNoColor, tempArg); } else if (typeof tempArg === 'bigint') { tempStr = `${tempArg}n`; + } else if (typeof tempArg !== 'object' || + tempArg === null || + !hasBuiltInToString(tempArg)) { + tempStr = String(tempArg); } else { - let constr; - if (typeof tempArg !== 'object' || - tempArg === null || - (typeof tempArg.toString === 'function' && - // A direct own property. - (ObjectPrototypeHasOwnProperty(tempArg, 'toString') || - // A direct own property on the constructor prototype in - // case the constructor is not an built-in object. - ((constr = tempArg.constructor) && - !builtInObjects.has(constr.name) && - constr.prototype && - ObjectPrototypeHasOwnProperty(constr.prototype, - 'toString'))))) { - tempStr = String(tempArg); - } else { - tempStr = inspect(tempArg, { - ...inspectOptions, - compact: 3, - colors: false, - depth: 0 - }); - } + tempStr = inspect(tempArg, { + ...inspectOptions, + compact: 3, + colors: false, + depth: 0 + }); } break; case 106: // 'j' diff --git a/test/parallel/test-util-format.js b/test/parallel/test-util-format.js index 2ef05284902995..e07ec6d6a34c2f 100644 --- a/test/parallel/test-util-format.js +++ b/test/parallel/test-util-format.js @@ -160,6 +160,66 @@ assert.strictEqual(util.format('%s', () => 5), '() => 5'); util.format('%s', new Foobar(5)), 'Foobar [ <5 empty items>, aaa: true ]' ); + + // Subclassing: + class B extends Foo {} + + function C() {} + C.prototype.toString = function() { + return 'Custom'; + }; + + function D() { + C.call(this); + } + D.prototype = Object.create(C.prototype); + + assert.strictEqual( + util.format('%s', new B()), + 'Bar' + ); + assert.strictEqual( + util.format('%s', new C()), + 'Custom' + ); + assert.strictEqual( + util.format('%s', new D()), + 'Custom' + ); + + D.prototype.constructor = D; + assert.strictEqual( + util.format('%s', new D()), + 'Custom' + ); + + D.prototype.constructor = null; + assert.strictEqual( + util.format('%s', new D()), + 'Custom' + ); + + D.prototype.constructor = { name: 'Foobar' }; + assert.strictEqual( + util.format('%s', new D()), + 'Custom' + ); + + Object.defineProperty(D.prototype, 'constructor', { + get() { + throw new Error(); + }, + configurable: true + }); + assert.strictEqual( + util.format('%s', new D()), + 'Custom' + ); + + assert.strictEqual( + util.format('%s', Object.create(null)), + '[Object: null prototype] {}' + ); } // JSON format specifier From 2f480411a6717253e24fc201fd7d266e6ede2452 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 26 Nov 2019 19:38:01 +0100 Subject: [PATCH 2/2] fixup --- lib/internal/util/inspect.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index ec4e02751a6ce3..45b92fdc92eb0f 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -1578,7 +1578,7 @@ function hasBuiltInToString(value) { } // The object has a own `toString` property. Thus it's not not a built-in one. - if (hasOwnProperty(value, 'toString')) { + if (ObjectPrototypeHasOwnProperty(value, 'toString')) { return false; } @@ -1587,7 +1587,7 @@ function hasBuiltInToString(value) { let pointer = value; do { pointer = ObjectGetPrototypeOf(pointer); - } while (!hasOwnProperty(pointer, 'toString')); + } while (!ObjectPrototypeHasOwnProperty(pointer, 'toString')); // Check closer if the object is a built-in. const descriptor = ObjectGetOwnPropertyDescriptor(pointer, 'constructor');