src: log warning on process.binding('natives') #2741

bnoordhuis · 2015-09-08T19:11:39Z

This is used by old versions of graceful-fs but will stop working in the
not too distant future. Print a warning so people know to upgrade.

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

thefourtheye · 2015-09-08T19:38:50Z

src/node.cc

+      // graceful-fs < 4 evals process.binding('natives').fs, which prohibits
+      // using internal modules in that module.  Encourage people to upgrade.
+      fprintf(stderr, "(node) process.binding('natives') is deprecated.\n");
+      fprintf(stderr, "(node) If you use graceful-fs < 4, please update.\n");


Should we really say this in the warning?

It's the sole reason for this PR.

We don't know if other modules do the same. Why should we care only about graceful-fs?

Because it's probably the most used offender. The only other projects I could find with Github's (admittedly terrible) code search is an sys / util shim (which does seem to get copied around a lot, there's that.)

While I dislike calling out a specific module here, graceful-fs is really heavily used and is causing specific problems here. Perhaps the compromise is to leave the note in but put a //TODO: Remove by v6 or similar in there just to mark it as a temporary message?

brycebaril · 2015-09-08T19:47:04Z

graceful-fs is likely the most-common but definitely not the only thing:

http://searchnod.es/#!binding(%22natives%22)/1

thefourtheye · 2015-09-08T19:50:06Z

src/node.cc

+        node::Utf8Value name(env->isolate(), frame->GetScriptName());
+        fprintf(stderr, "(node)    at %s:%d\n", *name, frame->GetLineNumber());
+      }
+      fflush(stderr);


stderr is not buffered right? Should we really flush it?

libc I/O can be buffered, that's why the fflush() is there.

brendanashworth · 2015-09-09T00:27:35Z

This kind of kicks the can down the road with process.binding, right? Is there any feasible way we can do this with internal modules?

vkurchatkin · 2015-09-09T09:15:10Z

I have an idea how we can deprecate public access to process.binding

bnoordhuis · 2015-09-09T09:30:45Z

For background, the reason for this PR is that I want to pre-compile our JS code using the V8 extras infrastructure. At that point process.binding('natives') can be removed if it weren't for modules like graceful-fs.

Fishrock123 · 2015-09-15T14:50:47Z

I think I'm ok with this. Making guarantees about our source being exposed at run-time is a ludicrous idea.

jasnell · 2015-09-25T16:19:13Z

LGTM.

bnoordhuis · 2015-09-26T18:22:02Z

Thanks @jasnell. I plan to merge this early next week so if anyone has strong feelings, now is the time to speak up.

Fishrock123 · 2015-09-26T18:24:21Z

I have strong feelings about this being a good idea. 👍

vkurchatkin · 2015-09-26T18:30:39Z

@bnoordhuis I propose to use #2768 for this. It's not necessary to deprecate whole process.binding right away, but we can use introduced hooks to deprecate natives in javascript. Also someone could just replace natives with $natives to suppress warning. This is not possible with #2768

bnoordhuis · 2015-09-26T19:07:56Z

Fine by me, as long as it gets done. Closing.

ChALkeR · 2016-08-24T22:24:24Z

@bnoordhuis It looks like it was actually never done, and #2768 was closed a few days ago.

Perhaps we should reopen this?

jasnell · 2016-08-25T16:52:22Z

If this goes forward it should be updated to make use of the emitWarning() API.

This is used by old versions of graceful-fs but will stop working in the not too distant future. Print a warning so people know to upgrade.

bnoordhuis · 2016-08-25T19:29:22Z

Rebased. Added a second commit that takes a different approach: check if the topmost JS frame is a builtin script.

If this goes forward it should be updated to make use of the emitWarning() API.

Why? We don't do that for other warnings in C++ code either.

ChALkeR · 2016-08-25T19:39:43Z

src/node.cc

+      fprintf(stderr,
+              "(node:%d) process.binding('natives') is deprecated.\n", pid);
+      fprintf(stderr,
+              "(node:%d) If you use graceful-fs < 4, please update.\n", pid);


~~Do recent versions of graceful-fs@3 still trigger this? It was updated just recently.~~
Update: they do, I verified that.

Another question: do we need to specifically note graceful-fs here? It's not the only package that uses process.binding('natives'), and that could lead people away from the actual issue.

Flashback: #2741 (comment)

@bnoordhuis yes, but see the list at #2741 (comment), that wasn't considered back then.

Also, graceful-fs@2 and graceful-fs@3 usage significantly decreased in past months.

I guess I could check if the caller is graceful-fs and customize the error based on that but meh. I have to be selective with my time in the next few weeks so I'm probably not going to expend too much effort on this PR.

If you don't have the time to work on it, I am willing to take it over.

jasnell · 2016-08-25T19:43:26Z

Why? We don't do that for other warnings in C++ code either.

So that it can be consistently handled as a runtime deprecation just like any other deprecation warning we print. As illustrated by #8270, it's pretty trivial and has the built-in benefit of working with --throw-deprecation.

Update ... for instance, with the patch as it is currently:

$ ./node --throw-deprecation
> const n = process.binding('natives')
(node:39039) process.binding('natives') is deprecated.
(node:39039) If you use graceful-fs < 4, please update.
(node:39039)    at repl:1
(node:39039)    at vm.js:22
(node:39039)    at vm.js:96
(node:39039)    at vm.js:21
(node:39039)    at repl.js:313
(node:39039)    at domain.js:280
(node:39039)    at domain.js:293
(node:39039)    at repl.js:504
(node:39039)    at events.js:101
(node:39039)    at events.js:188
(node:39039)    at readline.js:232
(node:39039)    at readline.js:572
undefined
>

Using emitWarning(), the warning would obey the --trace-deprecation and --throw-deprecation command line flags without any additional effort, and this PR could be simplified (without the extraneous (node:pid} outputs as well.

ChALkeR · 2016-08-25T19:52:29Z

I found 268 lines in 147 packages using process.binding('natives'): https://gist.github.com/ChALkeR/d8c9ff68869a1d86a3d702ce5c6d3a0a.

103 lines of that are require(process.binding('natives').util ? 'util' : 'sys') with syntaxical differences.
32 lines of the remaining are inside tests.
Without that — there are 133 files in 114 packages.

ChALkeR · 2016-08-25T20:05:21Z

src/node.cc

+    auto caller_script_id = GetCallerScriptId(env->isolate());
+    if (!IsInternalScriptId(env, caller_script_id)) {
+      // graceful-fs < 4 evals process.binding('natives').fs, which prohibits
+      // using internal modules in that module.  Encourage people to upgrade.


which prohibits using internal modules in that module.

This is not accurate any more, they evaded that, and it now reevalutes internal modules with more dark magic, internal API usage, and relying on implementation details.

bnoordhuis · 2016-08-25T20:18:35Z

Is this intentional?

Yes. Please be specific if you think there is something unintentional in there.

ChALkeR · 2016-08-25T20:21:56Z

@bnoordhuis Ok, I meant that the warning looks like too heavy to me, and most of those is cosumed by lines that do not bring much value (i.e. at module.js:…).

ChALkeR · 2016-08-25T20:26:42Z

Btw, one more thing: 4 packages actually assign to process.binding('natives')[something], and they succeed in that. That happens inside tests only, though.

Could we stop that, like now, without waiting for a whole major release cycle? This shouldn't get misused even more.

vkurchatkin · 2016-08-25T20:32:29Z

Could we stop that, like now, without waiting for a whole major release cycle?

That would be nice, making props non-writable and non-configurable should be enough

jasnell · 2016-08-25T21:03:52Z

Could we stop that

+1 ... especially if we know we can do it without breaking non-test code.

bnoordhuis · 2016-08-25T21:24:45Z

I meant that the warning looks like too heavy to me, and most of those is cosumed by lines that do not bring much value (i.e. at module.js:…).

I added the stack trace based on prior experience that if you don't, people are going to complain it's impossible to find out what is causing the warning.

ChALkeR · 2016-08-25T22:04:10Z

I added the stack trace based on prior experience that if you don't, people are going to complain it's impossible to find out what is causing the warning.

Makes sense. But could we strip out lines that match \s(internal/)?module\.js:? Those are pointing to internal files, if I understand things correctly.

bnoordhuis · 2016-08-26T08:19:51Z

Added a commit that does that (but in a better way than pattern matching strings.)

ChALkeR · 2016-08-26T10:22:15Z

@bnoordhuis Thanks!

Upd: yes, the warning looks good now.

ChALkeR · 2016-08-26T20:52:23Z

@bnoordhuis Sorry, I have another warning message stylistic question. How much extra code would it be to print the If you use graceful-fs < 4, please update. line only for those cases where graceful-fs is present in the backtrace? That would remove possible confusion for those users that get this warning for reasons other than graceful-fs.

Upd: e.g. webpack does that on it's own, without graceful-fs. I can't guarantee that that exact code is commonly triggered, but webpack has almost 2 millon downloads per month. It's not an argument against this PR (the fix for webpack is straightforward), it's an argument for not confusing those users with graceful-fs specific mesages.

Btw — is this semver-patch or semver-major?

jasnell · 2016-08-26T21:16:55Z

I'm -1 on this as is given that it is printed as a deprecation but does not properly follow the --trace-deprecation and --throw-deprecation command line flags. The behavior is inconsistent when it simply needn't be.

ChALkeR · 2016-08-26T21:36:24Z

@jasnell, would you change your mind if this would follow the command-line flags?

jasnell · 2016-08-26T22:17:56Z

Yep... see #2741 (comment)

jasnell · 2016-08-26T22:23:01Z

However, please keep in mind that the new fix for graceful-fs uses the natives module which uses process.binding('natives'), which is what has allowed us to get past the fs re-evaluation issue.

ChALkeR · 2016-08-26T22:29:36Z

@jasnell, older graceful-fs@3 versions used process.binding('natives') likewise, nothing has changed on that part. Btw, graceful-fs@4 should be fine, and most users have already upgraded to that.

jasnell · 2016-08-26T22:30:21Z

Yep, just wanted to make sure it was noted.

bnoordhuis · 2016-08-26T23:25:29Z

I'm -1 on this as is given that it is printed as a deprecation but does not properly follow the --trace-deprecation and --throw-deprecation command line flags. The behavior is inconsistent when it simply needn't be.

The JS API has no way to do the internal stack frame filtering that @ChALkeR requested, and besides, other deprecation warnings from C++ (e.g. deprecated Template::Set usage) don't use it either.

I could argue the PR in its current shape is more consistent with existing practices than it would be if I included your suggestion.

jasnell · 2016-08-27T01:11:12Z

At the very least --throw-deprecation and --trace-deprecation should be supported here in order for it to be a proper runtime deprecation... regardless of whether it uses emitWarning().

ChALkeR · 2016-08-27T04:32:43Z

@bnoordhuis

The JS API has no way to do the internal stack frame filtering that @ChALkeR requested

Just a note: I would be fine with not filtering if printing the trace would be behind a flag. I don't have any specific opinion on printing from js vs printing from the C++ side here at the moment, though.

thefourtheye reviewed Sep 8, 2015
View reviewed changes

mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 8, 2015

bnoordhuis closed this Sep 26, 2015

Fishrock123 mentioned this pull request Oct 10, 2015

Expose up-to-date list of builtin modules #3307

Closed

vkurchatkin mentioned this pull request Oct 16, 2015

node: deprecate public access to process.binding #2768

Closed

bnoordhuis reopened this Aug 25, 2016

jasnell mentioned this pull request Aug 25, 2016

src: make it simple to emit process warnings from src #8270

Closed

2 tasks

bnoordhuis added 2 commits August 25, 2016 21:21

src: log warning on process.binding('natives')

f51b943

This is used by old versions of graceful-fs but will stop working in the not too distant future. Print a warning so people know to upgrade.

squash! alternative take without $natives

35a62c5

bnoordhuis force-pushed the warn-on-process-binding-natives branch from 6eab98e to 35a62c5 Compare August 25, 2016 19:22

ChALkeR reviewed Aug 25, 2016
View reviewed changes

squash! do not print internal stack frames

dc8e0e6

ljharb mentioned this pull request Aug 26, 2016

Remove process.binding('natives') airbnb/hypernova#33

Open

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 27, 2016

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

bnoordhuis closed this Oct 25, 2016

leonardoadame mentioned this pull request Jul 8, 2023

[Snyk] Security upgrade semantic-release from 20.1.3 to 21.0.2 leonardoadame/node#96

Open

aliscco mentioned this pull request Jul 9, 2023

[Snyk] Security upgrade semantic-release from 17.4.7 to 21.0.2 aliscco/alisco-node#627

Open

leonardoadame mentioned this pull request May 13, 2024

[Snyk] Security upgrade semantic-release from 20.1.3 to 21.0.0 leonardoadame/node#816

Open

aliscco mentioned this pull request May 14, 2024

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1459

Open

src: log warning on process.binding('natives') #2741

src: log warning on process.binding('natives') #2741

Conversation

bnoordhuis commented Sep 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brycebaril commented Sep 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanashworth commented Sep 9, 2015

vkurchatkin commented Sep 9, 2015

bnoordhuis commented Sep 9, 2015

Fishrock123 commented Sep 15, 2015

jasnell commented Sep 25, 2015

bnoordhuis commented Sep 26, 2015

Fishrock123 commented Sep 26, 2015

vkurchatkin commented Sep 26, 2015

bnoordhuis commented Sep 26, 2015

ChALkeR commented Aug 24, 2016 • edited Loading

jasnell commented Aug 25, 2016

bnoordhuis commented Aug 25, 2016

ChALkeR Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChALkeR Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 25, 2016 • edited Loading

ChALkeR commented Aug 25, 2016 • edited Loading

ChALkeR Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

bnoordhuis commented Aug 25, 2016

ChALkeR commented Aug 25, 2016

ChALkeR commented Aug 25, 2016 • edited Loading

vkurchatkin commented Aug 25, 2016

jasnell commented Aug 25, 2016

bnoordhuis commented Aug 25, 2016

ChALkeR commented Aug 25, 2016 • edited Loading

bnoordhuis commented Aug 26, 2016

ChALkeR commented Aug 26, 2016 • edited Loading

ChALkeR commented Aug 26, 2016 • edited Loading

jasnell commented Aug 26, 2016

ChALkeR commented Aug 26, 2016

jasnell commented Aug 26, 2016

jasnell commented Aug 26, 2016

ChALkeR commented Aug 26, 2016

jasnell commented Aug 26, 2016

bnoordhuis commented Aug 26, 2016

jasnell commented Aug 27, 2016

ChALkeR commented Aug 27, 2016

ChALkeR commented Aug 24, 2016 •

edited

Loading

ChALkeR Aug 25, 2016 •

edited

Loading

ChALkeR Aug 25, 2016 •

edited

Loading

jasnell commented Aug 25, 2016 •

edited

Loading

ChALkeR commented Aug 25, 2016 •

edited

Loading

ChALkeR Aug 25, 2016 •

edited

Loading

ChALkeR commented Aug 25, 2016 •

edited

Loading

ChALkeR commented Aug 25, 2016 •

edited

Loading

ChALkeR commented Aug 26, 2016 •

edited

Loading

ChALkeR commented Aug 26, 2016 •

edited

Loading