src: refactor and harden `ProcessEmitWarning()` #17420

addaleax · 2017-12-01T23:14:19Z

Handle exceptions when getting process.emitWarning or when
calling it properly
Add Maybe<bool> to the return type, like the V8 API uses it
to indicate failure conditions
Update call sites to account for that and clean up/return to JS
when encountering an error
Add an internal ProcessEmitDeprecationWarning() sibling
for use in domain: runtime deprecate MakeCallback #17417,
with common code extracted to a helper function
Allow the warning to contain non-Latin-1 characters. Since the
message will usually be a template string containing data passed
from the user, this is the right thing to do.
Add tests for the failure modes (except string creation failures)
and UTF-8 warning messages.

Refs: #17417

Checklist

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

Affected core subsystem(s)

src

AndreasMadsen · 2017-12-01T23:19:53Z

src/node.cc

+
+  // MakeCallback() unneeded because emitWarning is internal code, it calls
+  // process.emit('warning', ...), but does so on the nextTick.
+  if (emit_warning.As<Function>()->Call(env->context(),


If process.emitWarning is supposed to be overwritable, as your test says. Then we should use MakeCallback, right?

Mh … I am not sure. There is usually always a JS call stack below, right?

Well, that won't be true for the domain MakeCallback deprecation. Anything could call MakeCallback in that case.

Hm. True. But that would put us in a pretty difficult position if process.domain happens to be set in that case (by accident), since process would be the resource object?

So, I am okay with changing this but I’m not sure where we’d get the async context to use for MakeCallback?

So, I am okay with changing this but I’m not sure where we’d get the async context to use for MakeCallback?

Yes. This is a general problem. We have process.on('beforeExit'), process.on('exit') without context as well. In this case I think there are two options:

Have a general async_context for the process, and use that.

Create a new async_context with async_context_.async_id as the triggerAsyncId.

The latter option is better for this case but I don't see it working in general for all ProcessEmitWarningGeneric use cases.

AndreasMadsen · 2017-12-01T23:27:05Z

src/node.cc

+  if (type != nullptr) {
+    if (!String::NewFromOneByte(env->isolate(),
+                              reinterpret_cast<const uint8_t*>(type),
+                              v8::NewStringType::kInternalized)


Question: what is an internalized string. Looking it up, I get something about string interning but that doesn't make much sense in this context.

It is basically what string interning refers to. As far as I know, what it means is that if V8 already has a copy of that same string, it deduplicates them and returns the existing one to the caller; And if not, the string is created in the “old space” part of the heap, that is, not expected to be short-lived and garbage-collected soon.

As a rule of thumb, this makes sense for things like identifier strings, I think.

Okay, that was my understanding of interning too :) It makes sense for type then, but not code as we are unlikely to emit the same deprecation warning twice.

Still – if there is e.g. code that tests the warning code, then that code will contain a copy of that string, right? Also, this is part of the generic helper, and I would like to add codes to the existing native warnings in a follow-up (because I think that might be semver-major)

Still – if there is e.g. code that tests the warning code, then that code will contain a copy of that string, right?

Yes.

Also, this is part of the generic helper, and I would like to add codes to the existing native warnings in a follow-up.

But those will be different codes. Even if it is the same deprecation code from multiple places, it should only be emitted once, thus the V8 string will only be constructed once.

If I understand interning correctly. The benefit isn't so much string copying, but rather string comparison. Instead of a copy, we end up doing some hash table lookup. Which is at least as costly, most likely more costly.

I would even go as far and say that interning the type only makes sense if the string object is reused. Such that it doesn't have to be reconstructed so often. In this case, it isn't reused, but we should properly do that.

Just to clarify: I really don't care about this, I'm just trying to learn :D

Even if it is the same deprecation code from multiple places, it should only be emitted once, thus the V8 string will only be constructed once.

Sorry, I was talking about the existing non-deprecation warnings (e.g. the crypto warning that is used in the test here :)

The benefit isn't so much string copying, but rather string comparison.

Yes, I would say so :) That’s why I usually go with using this for identifiers as a rule of thumb

bnoordhuis · 2017-12-02T13:03:01Z

src/node.cc

+
+  if (!emit_warning->IsFunction()) return Just(false);
+
+  int i = 0;


Suggestion: call this argc.

bnoordhuis · 2017-12-02T13:06:50Z

src/node_crypto.cc

+                               "load failed: %s\n",
+                               extra_root_certs_file.c_str(),
+                               ERR_error_string(err, nullptr)).IsNothing()) {
+          return;


This return screws up root_cert_store's reference counting

Hm – how would this fail? Returning here leaves root_cert_store initialized but unused, but the code doesn’t look to me like that’s harmful?

SecureContext::ctx_ won't have the root certificate store associated with it if you return here. That's a change from before.

bnoordhuis · 2017-12-02T13:07:57Z

src/node_crypto.cc

-                       cipher_type);
+    if (ProcessEmitWarning(env(), "Use Cipheriv for counter mode of %s",
+                           cipher_type).IsNothing())
+      return;


Likewise, this leaves the cipher object in a half-initialized state.

bnoordhuis · 2017-12-02T13:08:06Z

src/node_internals.h

+v8::Maybe<bool> ProcessEmitWarning(Environment* env, const char* fmt, ...);
+v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
+                                          const char* warning,
+                                          const char* deprecation_code);


bnoordhuis · 2017-12-02T13:09:33Z

test/parallel/test-tls-env-bad-extra-ca.js

@@ -18,7 +18,7 @@ if (process.env.CHILD) {

 const env = Object.assign({}, process.env, {
  CHILD: 'yes',
-  NODE_EXTRA_CA_CERTS: `${fixtures.fixturesDir}/no-such-file-exists`,
+  NODE_EXTRA_CA_CERTS: `${fixtures.fixturesDir}/no-such-file-exists-🐢`,


bnoordhuis · 2017-12-02T13:13:46Z

src/node.cc

+  if (type != nullptr) {
+    if (!String::NewFromOneByte(env->isolate(),
+                                reinterpret_cast<const uint8_t*>(type),
+                                v8::NewStringType::kInternalized)


I don't know if it makes sense to intern the strings - they stay around forever but they're probably used only once. (At least, how often is the same warning emitted? Not too often, I'd guess.)

You and @AndreasMadsen convinced me :)

AndreasMadsen

Mostly LGTM

I'm fine with using ->Call() for now. However, the comment should definitely be changed.

As said, we need to fix this on a more general level.

addaleax · 2017-12-09T14:37:12Z

Fresh CI: https://ci.nodejs.org/job/node-test-commit/14700/

apapirovski · 2017-12-09T20:17:03Z

@addaleax looks like there's at least one related failure in CI:

https://ci.nodejs.org/job/node-test-commit-linux-linked/nodes=ubuntu1604_sharedlibs_fips20_x64/728/tapResults/

internal/crypto/cipher.js:77
   this._handle.init(cipher, password);
                ^  
Error: crypto.createCipher() is not supported in FIPS mode.
     at new Cipher (internal/crypto/cipher.js:77:16)
     at Object.createCipher (crypto.js:94:10)
     at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-linked/nodes/ubuntu1604_sharedlibs_fips20_x64/test/parallel/test-process-emit-warning-from-native.js:16:10)
     at Module._compile (module.js:660:30)
     at Object.Module._extensions..js (module.js:671:10)
     at Module.load (module.js:577:32)
     at tryModuleLoad (module.js:517:12)
     at Function.Module._load (module.js:509:3)
     at Function.Module.runMain (module.js:701:10)
     at startup (bootstrap_node.js:195:16)

- Handle exceptions when getting `process.emitWarning` or when calling it properly - Add `Maybe<bool>` to the return type, like the V8 API uses it to indicate failure conditions - Update call sites to account for that and clean up/return to JS when encountering an error - Add an internal `ProcessEmitDeprecationWarning()` sibling for use in nodejs#17417, with common code extracted to a helper function - Allow the warning to contain non-Latin-1 characters. Since the message will usually be a template string containing data passed from the user, this is the right thing to do. - Add tests for the failure modes (except string creation failures) and UTF-8 warning messages. Refs: nodejs#17417

addaleax · 2017-12-10T02:17:49Z

CI: https://ci.nodejs.org/job/node-test-commit/14716/

BridgeAR

JS part is LGTM

BridgeAR · 2017-12-12T19:48:11Z

Landed in f3cd537

- Handle exceptions when getting `process.emitWarning` or when calling it properly - Add `Maybe<bool>` to the return type, like the V8 API uses it to indicate failure conditions - Update call sites to account for that and clean up/return to JS when encountering an error - Add an internal `ProcessEmitDeprecationWarning()` sibling for use in nodejs#17417, with common code extracted to a helper function - Allow the warning to contain non-Latin-1 characters. Since the message will usually be a template string containing data passed from the user, this is the right thing to do. - Add tests for the failure modes (except string creation failures) and UTF-8 warning messages. PR-URL: nodejs#17420 Refs: nodejs#17417 Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

- Handle exceptions when getting `process.emitWarning` or when calling it properly - Add `Maybe<bool>` to the return type, like the V8 API uses it to indicate failure conditions - Update call sites to account for that and clean up/return to JS when encountering an error - Add an internal `ProcessEmitDeprecationWarning()` sibling for use in #17417, with common code extracted to a helper function - Allow the warning to contain non-Latin-1 characters. Since the message will usually be a template string containing data passed from the user, this is the right thing to do. - Add tests for the failure modes (except string creation failures) and UTF-8 warning messages. PR-URL: #17420 Refs: #17417 Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2018-01-22T08:42:01Z

Should this land on LTS? I believe there was a question about potentially backporting this to 6.x, but this may be needed for 8.x either way

MylesBorins · 2018-02-27T03:04:25Z

ping re: backport

MylesBorins · 2018-05-22T15:57:35Z

ping re: backport

addaleax requested a review from AndreasMadsen December 1, 2017 23:14

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 1, 2017

AndreasMadsen reviewed Dec 1, 2017

View reviewed changes

bnoordhuis reviewed Dec 2, 2017

View reviewed changes

AndreasMadsen approved these changes Dec 8, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

addaleax force-pushed the harden-process-emitwarning branch from e45d167 to d83f667 Compare December 9, 2017 14:35

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

apapirovski removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 9, 2017

addaleax added 4 commits December 10, 2017 01:19

[squash] indentation

657555d

[squash] bnoordhuis & AndreasMadsen nits

bb5ba30

[squash] bnoordhuis comment

1c72f29

addaleax force-pushed the harden-process-emitwarning branch from d83f667 to 12c4f38 Compare December 10, 2017 00:20

addaleax added 2 commits December 10, 2017 02:28

[squash] skip test in FIPS mode

35d3c42

[squash] skip environment variable encoding test on windows

a9613ac

addaleax force-pushed the harden-process-emitwarning branch from 12c4f38 to a9613ac Compare December 10, 2017 01:36

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

BridgeAR approved these changes Dec 12, 2017

View reviewed changes

BridgeAR closed this Dec 12, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added lts-watch-v6.x labels Jan 22, 2018

addaleax deleted the harden-process-emitwarning branch January 22, 2018 14:43

MylesBorins added backport-requested-v8.x and removed lts-watch-v6.x labels May 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: refactor and harden `ProcessEmitWarning()` #17420

src: refactor and harden `ProcessEmitWarning()` #17420

addaleax commented Dec 1, 2017

AndreasMadsen Dec 1, 2017 •

edited

Loading

addaleax Dec 1, 2017

AndreasMadsen Dec 1, 2017

addaleax Dec 1, 2017

AndreasMadsen Dec 2, 2017

AndreasMadsen Dec 1, 2017

addaleax Dec 1, 2017

AndreasMadsen Dec 1, 2017

addaleax Dec 1, 2017

AndreasMadsen Dec 2, 2017 •

edited

Loading

addaleax Dec 2, 2017

bnoordhuis Dec 2, 2017

bnoordhuis Dec 2, 2017

addaleax Dec 2, 2017

bnoordhuis Dec 3, 2017

bnoordhuis Dec 2, 2017

bnoordhuis Dec 2, 2017

bnoordhuis Dec 2, 2017

bnoordhuis Dec 2, 2017

addaleax Dec 2, 2017

AndreasMadsen left a comment

addaleax commented Dec 9, 2017

apapirovski commented Dec 9, 2017

addaleax commented Dec 10, 2017

BridgeAR left a comment

BridgeAR commented Dec 12, 2017

MylesBorins commented Jan 22, 2018

MylesBorins commented Feb 27, 2018

MylesBorins commented May 22, 2018


		if (!emit_warning->IsFunction()) return Just(false);

		int i = 0;

src: refactor and harden ProcessEmitWarning() #17420

src: refactor and harden ProcessEmitWarning() #17420

Conversation

addaleax commented Dec 1, 2017

Checklist

Affected core subsystem(s)

AndreasMadsen Dec 1, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen Dec 2, 2017 • edited Loading

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

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

AndreasMadsen left a comment

Choose a reason for hiding this comment

addaleax commented Dec 9, 2017

apapirovski commented Dec 9, 2017

addaleax commented Dec 10, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Dec 12, 2017

MylesBorins commented Jan 22, 2018

MylesBorins commented Feb 27, 2018

MylesBorins commented May 22, 2018

src: refactor and harden `ProcessEmitWarning()` #17420

src: refactor and harden `ProcessEmitWarning()` #17420

AndreasMadsen Dec 1, 2017 •

edited

Loading

AndreasMadsen Dec 2, 2017 •

edited

Loading