util: print External address from inspect #34398

rosaxxny · 2020-07-16T16:32:12Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2020-07-16T16:33:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32375/

juanarbol · 2020-07-17T06:34:48Z

src/node_util.cc

+static void GetExternalValue(
+    const FunctionCallbackInfo<Value>& args) {
+  CHECK(args[0]->IsExternal());
+  auto isolate = args.GetIsolate();


This is nice! Thank you!

This is not a blocker at all, but I'd prefer specify the type

Suggested change

auto isolate = args.GetIsolate();

Isolate* isolate = args.GetIsolate();

Just a sugestion, not a blocker.

tniessen · 2020-07-17T20:53:59Z

I think this always prints the pointer, unlike what @addaleax suggested in #28250 (comment). Anna expressed potential security concerns there.

addaleax · 2020-07-17T21:42:41Z

@tniessen Yeah, it’s … not simple. On the one hand, the pointer should not end up in the hands of an attacker – on the other hand, if you’re publicly sharing debugging information for internal objects, like util.inspect() returns, then you’re likely to expose more sensitive information than that anyway. So I think this is okay, but if somebody else feels strongly, I also understand that.

tniessen · 2020-07-17T23:27:53Z

I am not overly concerned either. Are there any objects returned by core APIs that contain externals that would expose internal pointers when util.inspect()ed? Without accessing undocumented properties? So does Node.js core return any objects that could previously safely be inspected, and might now expose pointers?

addaleax · 2020-07-17T23:51:29Z

@tniessen No, Node.js core should currently be fine anyway. We only use Externals for StreamBase and SecureContext, but these objects are only created through prototype getters, so neither are the property names printed in the default inspect output, nor are the External values actually created there.

tniessen

LGTM then, thank you for the explanation @addaleax.

bnoordhuis · 2020-07-18T08:32:09Z

src/node_util.cc

+
+  void* ptr = external->Value();
+  char val[20];
+  snprintf(val, sizeof(val), "%p", ptr);


%p can either format the pointer as 1234abcde or 0x1234abcd, and there's also potential inconsistency around NULL pointers. I'd cast the pointer to uint64_t and format it with PRIx64 from <cinttypes>.

An alternative solution is to turn the pointer into a BigInt with v8::BigInt::NewFromUnsigned() and do the formatting in JS land with value.toString(16).

bnoordhuis · 2020-07-18T08:34:49Z

test/parallel/test-util-inspect.js

-assert.strictEqual(util.inspect((new JSStream())._externalStream),
-                   '[External]');
+assert.match(util.inspect((new JSStream())._externalStream),
+             /^\[External: .*?\]$/);


Can you check that the value is a non-empty hex string?

Suggested change

/^\[External: .*?\]$/);

/^\[External: [0-9a-f]+\]$/);

rosaxxny · 2020-07-23T15:16:36Z

@juanarbol @bnoordhuis @tniessen Updated, PTAL.

Fixes: nodejs#28250

tniessen · 2020-07-23T16:37:09Z

Changes still LGTM, but the branch seems to have unrelated changes. Maybe a merge commit is the problem?

nodejs-github-bot · 2020-07-27T13:28:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32515/

addaleax · 2020-07-28T12:31:13Z

Landed in fe2a7f0

Fixes: #28250 PR-URL: #34398 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

ruyadorno · 2020-07-28T15:36:02Z

This does not land cleanly on v14.x, should it be backported?

Fixes: nodejs#28250 PR-URL: nodejs#34398 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Fixes: #28250 PR-URL: #34398 Backport-PR-URL: #34583 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Jul 16, 2020

addaleax approved these changes Jul 16, 2020

View reviewed changes

devsnek approved these changes Jul 16, 2020

View reviewed changes

juanarbol approved these changes Jul 17, 2020

View reviewed changes

jasnell approved these changes Jul 17, 2020

View reviewed changes

tniessen approved these changes Jul 18, 2020

View reviewed changes

bnoordhuis reviewed Jul 18, 2020

View reviewed changes

This was referenced Jul 21, 2020

CI Relability 2020-07-21 JaneaSystems/nodejs-reliability#44

Open

CI Relability 2020-07-22 JaneaSystems/nodejs-reliability#45

Open

rosaxxny added 3 commits July 23, 2020 12:09

util: print External address from inspect

9d071d9

Fixes: nodejs#28250

fixup! util: print External address from inspect

b0f6baa

fixup! fixup! util: print External address from inspect

880c5b1

rosaxxny requested a review from a team July 23, 2020 16:22

rosaxxny force-pushed the util/inspect-external branch from 0387c8d to 880c5b1 Compare July 23, 2020 23:45

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 27, 2020

addaleax closed this Jul 28, 2020

ruyadorno added the backport-requested-v14.x label Jul 28, 2020

rosaxxny mentioned this pull request Jul 31, 2020

[v14.x backport] util: print External address from inspect #34583

Closed

codebytere mentioned this pull request Aug 10, 2020

v14.8.0 proposal #34704

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

targos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v14.x labels Apr 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: print External address from inspect #34398

util: print External address from inspect #34398

rosaxxny commented Jul 16, 2020

nodejs-github-bot commented Jul 16, 2020

juanarbol Jul 17, 2020

tniessen commented Jul 17, 2020

addaleax commented Jul 17, 2020

tniessen commented Jul 17, 2020

addaleax commented Jul 17, 2020

tniessen left a comment

bnoordhuis Jul 18, 2020

bnoordhuis Jul 18, 2020

tniessen Jul 18, 2020

rosaxxny commented Jul 23, 2020

tniessen commented Jul 23, 2020

nodejs-github-bot commented Jul 27, 2020

addaleax commented Jul 28, 2020

ruyadorno commented Jul 28, 2020

	auto isolate = args.GetIsolate();
	Isolate* isolate = args.GetIsolate();

util: print External address from inspect #34398

util: print External address from inspect #34398

Conversation

rosaxxny commented Jul 16, 2020

Checklist

nodejs-github-bot commented Jul 16, 2020

juanarbol Jul 17, 2020

Choose a reason for hiding this comment

tniessen commented Jul 17, 2020

addaleax commented Jul 17, 2020

tniessen commented Jul 17, 2020

addaleax commented Jul 17, 2020

tniessen left a comment

Choose a reason for hiding this comment

bnoordhuis Jul 18, 2020

Choose a reason for hiding this comment

bnoordhuis Jul 18, 2020

Choose a reason for hiding this comment

tniessen Jul 18, 2020

Choose a reason for hiding this comment

rosaxxny commented Jul 23, 2020

tniessen commented Jul 23, 2020

nodejs-github-bot commented Jul 27, 2020

addaleax commented Jul 28, 2020

ruyadorno commented Jul 28, 2020