util: print External address from inspect

[rosaxxny]

Fixes: #28250

Checklist

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

[nodejs-github-bot]

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

[juanarbol]

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]

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

[addaleax]

@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]

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]

@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]

%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]

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

[tniessen]

Suggested change

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

[rosaxxny]

@juanarbol @bnoordhuis @tniessen Updated, PTAL.

[tniessen]

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

[nodejs-github-bot]

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

[addaleax]

Landed in fe2a7f0

[ruyadorno]

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