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

Hardened JavaScript interferes with Node.js 14 Error construction #868

Closed
kriskowal opened this issue Aug 24, 2021 · 6 comments
Closed

Hardened JavaScript interferes with Node.js 14 Error construction #868

kriskowal opened this issue Aug 24, 2021 · 6 comments
Assignees

Comments

@kriskowal
Copy link
Member

@warner reports:

Then I used fs.openReadStream(undefined) (not intentionally, I forgot to provide argv). The error I get is TypeError#1: Cannot assign to read only property 'name' of object 'TypeError: The "path" argument must be of type string or an instance of Buffer or URL. Received undefined'
the stack trace includes:

 at addCodeToName (internal/errors.js:329:12)
 at new NodeError (internal/errors.js:294:7)
 at internal/fs/utils.js:634:11

The relevant code in Node.js 14.17.4 is:

function addCodeToName(err, name, code) {
  // Set the stack
  if (excludedStackFn !== undefined) {
    ErrorCaptureStackTrace(err, excludedStackFn);
  }
  // Add the error code to the name to include it in the stack trace.
  err.name = `${name} [${code}]`;
  // Access the stack to generate the error message including the error code
  // from the name.
  err.stack; // eslint-disable-line no-unused-expressions
  // Reset the name to the actual name.
  if (name === 'SystemError') {
    ObjectDefineProperty(err, 'name', {
      value: name,
      enumerable: false,
      writable: true,
      configurable: true
    });
  } else {
    delete err.name;
  }
}

And the culprit is the property override hazard, upon assigning to name of an instance descended from a frozen error constructor.

@erights
Copy link
Contributor

erights commented Aug 24, 2021

Although #867 fixes the issue revealed here, it won't actually change this specific occurrence. The problematic Node code might override the name property of any error by assignment. #867 extends the default overrideTaming: 'moderate' setting to enable override-by-assignment of name for all error types. But it was already enabled for Error and TypeError, so this symptom would not have been triggered at the 'moderate' setting. We only became aware of it by debugging at the overrideTaming: 'min' setting. At that setting, this problem will still occur.

@Tartuffo
Copy link

Tartuffo commented Feb 2, 2022

@kriskowal Can you please put an estimate on this?

@kriskowal
Copy link
Member Author

@erights I’m not clear on whether or what the next steps for this issue would be. Is there fix we could stomach that would address this issue at all overrideTaming settings?

@Tartuffo Tartuffo added the MN-1 label Feb 2, 2022
@erights
Copy link
Contributor

erights commented Feb 3, 2022

Well, we could trivially copy this moderate change so it applies to min too. I don't like it, but Node....

Have we told Node about this? Have we filed an issue? Is it still a problem on Node 16?

@Tartuffo
Copy link

Tartuffo commented Feb 3, 2022

@kriskowal This does not have an area label that is covered by our weekly tech / planning meetings. Can you assign the proper label? We cover: agd, agoric-cosmos, amm, core economy, cosmic-swingset, endo, getrun, governance, installation-bundling, metering, run-protocol, staking, swingset, swingset-runner, token economy, wallet, zoe contract,

@kriskowal kriskowal added the ses label Feb 3, 2022
@Tartuffo Tartuffo removed the MN-1 label Feb 7, 2022
@kriskowal
Copy link
Member Author

We no longer support Node.js 14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants