Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: throw when using public methods instead of primordials #36164

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 18, 2020

This PR adds a check on our tests to unsure core doesn't use any built-in methods from the global object which could be mitigated by users.

Simple example

' '.repeat(4)

If that line of code is inside an internal module, it would raise the following error:

Error: Unsafe use of `<object>.repeat(...<arguments>)`. Use `primordials.StringPrototypeRepeat(<object>, ...<arguments>)` instead, or `callUnsafe(<object>, 'repeat', ...<arguments>)`.

When we meant to use the global builtins

Sometimes, we really want to call the user-mitigated methods. A typical example are Thenables, where we usually want to support third-party libraries, and not always call Promise.prototype.then. It's usually done like this:

const then = promise.then;
if (typeof then === 'function') then.call(promise, onSuccess, onError);

That would raise two errors:

Error: Unsafe use of `<object>.call(...<arguments>)`. Use `primordials.ReflectApply(<object>, ...<arguments>)` instead, or `callUnsafe(<object>, 'call', ...<arguments>)`.
Error: Unsafe use of `<object>.then(...<arguments>)`. Use `primordials.PromisePrototypeThen(<object>, ...<arguments>)` instead, or `callUnsafe(<object>, 'then', ...<arguments>)`.

The correct™️ way of doing that would indeed be to use callUnsafe:

callUnsafe(promise, 'then', onSuccess, onError);

or

const then = new CallUnsafe(promise, 'then');
if(then.is_callable) then.call(onSuccess, onError);
else { /* Do something else */ }

Limitations

  • Unlike a linter check, there is no guarantee this tool can check all unsafe uses of JS builtins. That limitation would go away with a test coverage of 100%.
  • I'm using Error#stack to test if the call was made by an internal or by the test files. It's detecting node: prefix, which was introduced in Node.js 15. I don't think this can be backported to earlier release lines.
  • There are SO MANY failing tests currently. Changing all internal calls to primordials is an enormous work, however I'm confident it's feasible, maybe by encouraging first-time contributors to pick up certain files. It's a great way of diving into Node.js internals, which helps to better understand how it's working and help gain confidence to contribute further on the code base.

Next steps

We might want to land a modified version without waiting for all the tests to pass:

  • Make the remove-primordials module opt-in.
  • Emit warnings instead of throwing errors.
  • Have a tool that counts the number of warning to make sure the number is getting down (I.E. we are not introducing more unsafe calls to internals).

Fixes: #30697

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration of core modules to primordials
3 participants