From 0bca959617f3ae63e548610a97b9ab47bde97710 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 29 Apr 2016 01:06:48 -0700 Subject: [PATCH] util: fix inspecting of proxy objects In certain conditions, inspecting a Proxy object can lead to a max call stack error. Avoid that by detecting the Proxy object and outputting information about the Proxy object itself. Fixes: https://github.com/nodejs/node/issues/6464 PR-URL: https://github.com/nodejs/node/pull/6465 Reviewed-By: Myles Borins Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis --- benchmark/util/inspect-proxy.js | 44 +++++++++++++ doc/api/util.md | 4 ++ lib/util.js | 42 +++++++++++- src/node_util.cc | 16 +++++ test/parallel/test-util-inspect-proxy.js | 84 ++++++++++++++++++++++++ 5 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 benchmark/util/inspect-proxy.js create mode 100644 test/parallel/test-util-inspect-proxy.js diff --git a/benchmark/util/inspect-proxy.js b/benchmark/util/inspect-proxy.js new file mode 100644 index 00000000000000..92d5a9a47d6dcb --- /dev/null +++ b/benchmark/util/inspect-proxy.js @@ -0,0 +1,44 @@ +'use strict'; + +const util = require('util'); +const common = require('../common.js'); + +const bench = common.createBenchmark(main, { + v: [1, 2], + n: [1e6] +}); + +function twoDifferentProxies(n) { + // This one should be slower between we're looking up multiple proxies. + const proxyA = new Proxy({}, {get: () => {}}); + const proxyB = new Proxy({}, {get: () => {}}); + bench.start(); + for (var i = 0; i < n; i += 1) + util.inspect({a: proxyA, b: proxyB}, {showProxy: true}); + bench.end(n); +} + +function oneProxy(n) { + // This one should be a bit faster because of the internal caching. + const proxy = new Proxy({}, {get: () => {}}); + bench.start(); + for (var i = 0; i < n; i += 1) + util.inspect({a: proxy, b: proxy}, {showProxy: true}); + bench.end(n); +} + +function main(conf) { + const n = conf.n | 0; + const v = conf.v | 0; + + switch (v) { + case 1: + oneProxy(n); + break; + case 2: + twoDifferentProxies(n); + break; + default: + throw new Error('Should not get to here'); + } +} diff --git a/doc/api/util.md b/doc/api/util.md index 7855bfe8b40800..8ef4b0dfcc564d 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -179,6 +179,10 @@ formatted string: - `customInspect` - if `false`, then custom `inspect(depth, opts)` functions defined on the objects being inspected won't be called. Defaults to `true`. + - `showProxy` - if `true`, then objects and functions that are Proxy objects + will be introspected to show their `target` and `hander` objects. Defaults to + `false`. + Example of inspecting all properties of the `util` object: ```js diff --git a/lib/util.js b/lib/util.js index 63d6d0f3c865af..69df2501997309 100644 --- a/lib/util.js +++ b/lib/util.js @@ -139,6 +139,7 @@ function inspect(obj, opts) { if (ctx.depth === undefined) ctx.depth = 2; if (ctx.colors === undefined) ctx.colors = false; if (ctx.customInspect === undefined) ctx.customInspect = true; + if (ctx.showProxy === undefined) ctx.showProxy = false; if (ctx.colors) ctx.stylize = stylizeWithColor; return formatValue(ctx, obj, ctx.depth); } @@ -241,6 +242,46 @@ function inspectPromise(p) { function formatValue(ctx, value, recurseTimes) { + if (ctx.showProxy && + ((typeof value === 'object' && value !== null) || + typeof value === 'function')) { + var proxy = undefined; + var proxyCache = ctx.proxyCache; + if (!proxyCache) + proxyCache = ctx.proxyCache = new Map(); + // Determine if we've already seen this object and have + // determined that it either is or is not a proxy. + if (proxyCache.has(value)) { + // We've seen it, if the value is not undefined, it's a Proxy. + proxy = proxyCache.get(value); + } else { + // Haven't seen it. Need to check. + // If it's not a Proxy, this will return undefined. + // Otherwise, it'll return an array. The first item + // is the target, the second item is the handler. + // We ignore (and do not return) the Proxy isRevoked property. + proxy = binding.getProxyDetails(value); + if (proxy) { + // We know for a fact that this isn't a Proxy. + // Mark it as having already been evaluated. + // We do this because this object is passed + // recursively to formatValue below in order + // for it to get proper formatting, and because + // the target and handle objects also might be + // proxies... it's unfortunate but necessary. + proxyCache.set(proxy, undefined); + } + // If the object is not a Proxy, then this stores undefined. + // This tells the code above that we've already checked and + // ruled it out. If the object is a proxy, this caches the + // results of the getProxyDetails call. + proxyCache.set(value, proxy); + } + if (proxy) { + return 'Proxy ' + formatValue(ctx, proxy, recurseTimes); + } + } + // Provide a hook for user-specified inspect functions. // Check that value is an object with an inspect function on it if (ctx.customInspect && @@ -785,7 +826,6 @@ exports.isPrimitive = isPrimitive; exports.isBuffer = Buffer.isBuffer; - function pad(n) { return n < 10 ? '0' + n.toString(10) : n.toString(10); } diff --git a/src/node_util.cc b/src/node_util.cc index 1c5a2fa9a1f373..9fc94acf094426 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -6,11 +6,13 @@ namespace node { namespace util { +using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; using v8::Local; using v8::Object; using v8::Private; +using v8::Proxy; using v8::String; using v8::Value; @@ -37,6 +39,19 @@ using v8::Value; VALUE_METHOD_MAP(V) #undef V +static void GetProxyDetails(const FunctionCallbackInfo& args) { + // Return undefined if it's not a proxy. + if (!args[0]->IsProxy()) + return; + + Local proxy = args[0].As(); + + Local ret = Array::New(args.GetIsolate(), 2); + ret->Set(0, proxy->GetTarget()); + ret->Set(1, proxy->GetHandler()); + + args.GetReturnValue().Set(ret); +} static void GetHiddenValue(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -84,6 +99,7 @@ void Initialize(Local target, env->SetMethod(target, "getHiddenValue", GetHiddenValue); env->SetMethod(target, "setHiddenValue", SetHiddenValue); + env->SetMethod(target, "getProxyDetails", GetProxyDetails); } } // namespace util diff --git a/test/parallel/test-util-inspect-proxy.js b/test/parallel/test-util-inspect-proxy.js new file mode 100644 index 00000000000000..6311884b85b5f1 --- /dev/null +++ b/test/parallel/test-util-inspect-proxy.js @@ -0,0 +1,84 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); +const util = require('util'); +const processUtil = process.binding('util'); +const opts = {showProxy: true}; + +const target = {}; +const handler = { + get: function() { throw new Error('Getter should not be called'); } +}; +const proxyObj = new Proxy(target, handler); + +// Inspecting the proxy should not actually walk it's properties +assert.doesNotThrow(() => util.inspect(proxyObj, opts)); + +// getProxyDetails is an internal method, not intended for public use. +// This is here to test that the internals are working correctly. +const details = processUtil.getProxyDetails(proxyObj); +assert.strictEqual(target, details[0]); +assert.strictEqual(handler, details[1]); + +assert.strictEqual(util.inspect(proxyObj, opts), + 'Proxy [ {}, { get: [Function] } ]'); + +// Using getProxyDetails with non-proxy returns undefined +assert.strictEqual(processUtil.getProxyDetails({}), undefined); + +// This will throw because the showProxy option is not used +// and the get function on the handler object defined above +// is actually invoked. +assert.throws( + () => util.inspect(proxyObj) +); + +// Yo dawg, I heard you liked Proxy so I put a Proxy +// inside your Proxy that proxies your Proxy's Proxy. +const proxy1 = new Proxy({}, {}); +const proxy2 = new Proxy(proxy1, {}); +const proxy3 = new Proxy(proxy2, proxy1); +const proxy4 = new Proxy(proxy1, proxy2); +const proxy5 = new Proxy(proxy3, proxy4); +const proxy6 = new Proxy(proxy5, proxy5); +const expected0 = '{}'; +const expected1 = 'Proxy [ {}, {} ]'; +const expected2 = 'Proxy [ Proxy [ {}, {} ], {} ]'; +const expected3 = 'Proxy [ Proxy [ Proxy [ {}, {} ], {} ], Proxy [ {}, {} ] ]'; +const expected4 = 'Proxy [ Proxy [ {}, {} ], Proxy [ Proxy [ {}, {} ], {} ] ]'; +const expected5 = 'Proxy [ Proxy [ Proxy [ Proxy [Object], {} ],' + + ' Proxy [ {}, {} ] ],\n Proxy [ Proxy [ {}, {} ]' + + ', Proxy [ Proxy [Object], {} ] ] ]'; +const expected6 = 'Proxy [ Proxy [ Proxy [ Proxy [Object], Proxy [Object]' + + ' ],\n Proxy [ Proxy [Object], Proxy [Object] ] ],\n' + + ' Proxy [ Proxy [ Proxy [Object], Proxy [Object] ],\n' + + ' Proxy [ Proxy [Object], Proxy [Object] ] ] ]'; +assert.strictEqual(util.inspect(proxy1, opts), expected1); +assert.strictEqual(util.inspect(proxy2, opts), expected2); +assert.strictEqual(util.inspect(proxy3, opts), expected3); +assert.strictEqual(util.inspect(proxy4, opts), expected4); +assert.strictEqual(util.inspect(proxy5, opts), expected5); +assert.strictEqual(util.inspect(proxy6, opts), expected6); +assert.strictEqual(util.inspect(proxy1), expected0); +assert.strictEqual(util.inspect(proxy2), expected0); +assert.strictEqual(util.inspect(proxy3), expected0); +assert.strictEqual(util.inspect(proxy4), expected0); +assert.strictEqual(util.inspect(proxy5), expected0); +assert.strictEqual(util.inspect(proxy6), expected0); + +// Just for fun, let's create a Proxy using Arrays. +const proxy7 = new Proxy([], []); +const expected7 = 'Proxy [ [], [] ]'; +assert.strictEqual(util.inspect(proxy7, opts), expected7); +assert.strictEqual(util.inspect(proxy7), '[]'); + +// Now we're just getting silly, right? +const proxy8 = new Proxy(Date, []); +const proxy9 = new Proxy(Date, String); +const expected8 = 'Proxy [ [Function: Date], [] ]'; +const expected9 = 'Proxy [ [Function: Date], [Function: String] ]'; +assert.strictEqual(util.inspect(proxy8, opts), expected8); +assert.strictEqual(util.inspect(proxy9, opts), expected9); +assert.strictEqual(util.inspect(proxy8), '[Function: Date]'); +assert.strictEqual(util.inspect(proxy9), '[Function: Date]');