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

module: fix crash when built-in module export a default key #51481

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 16, 2024

Fixes: #51480

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 16, 2024
@guybedford
Copy link
Contributor

Looking at this bug, it seems that assigning arbitrary keys to process before importing it can affect its shape. For example:

process.asdf = 'blah';
const processMod = await import('node:process');
console.log(processMod.asdf); // blah

I would argue that this is still a bug even with this PR fix for the default crash.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course I approve the fix, just noting there is some wider inconsistency here.

lib/internal/bootstrap/realm.js Outdated Show resolved Hide resolved
Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 16, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 19, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51481
✔  Done loading data for nodejs/node/pull/51481
----------------------------------- PR info ------------------------------------
Title      module: fix crash when built-in module export a `default` key (#51481)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:process-default -> nodejs:main
Labels     lib / src, author ready, needs-ci
Commits    2
 - module: fix crash when built-in module export a `default` key
 - Update lib/internal/bootstrap/realm.js
Committers 2
 - Antoine du Hamel 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/51481
Fixes: https://github.com/nodejs/node/issues/51480
Reviewed-By: Chengzhong Wu 
Reviewed-By: Guy Bedford 
Reviewed-By: Geoffrey Booth 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51481
Fixes: https://github.com/nodejs/node/issues/51480
Reviewed-By: Chengzhong Wu 
Reviewed-By: Guy Bedford 
Reviewed-By: Geoffrey Booth 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - Update lib/internal/bootstrap/realm.js
   ℹ  This PR was created on Tue, 16 Jan 2024 00:18:21 GMT
   ✔  Approvals: 3
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/51481#pullrequestreview-1822478224
   ✔  - Guy Bedford (@guybedford): https://github.com/nodejs/node/pull/51481#pullrequestreview-1822522624
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/51481#pullrequestreview-1822572122
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-01-16T17:44:01Z: https://ci.nodejs.org/job/node-test-pull-request/56816/
- Querying data for job/node-test-pull-request/56816/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/7580563296

@legendecas legendecas added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 19, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 19, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51481
✔  Done loading data for nodejs/node/pull/51481
----------------------------------- PR info ------------------------------------
Title      module: fix crash when built-in module export a `default` key (#51481)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:process-default -> nodejs:main
Labels     lib / src, author ready, needs-ci
Commits    2
 - module: fix crash when built-in module export a `default` key
 - Update lib/internal/bootstrap/realm.js
Committers 2
 - Antoine du Hamel 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/51481
Fixes: https://github.com/nodejs/node/issues/51480
Reviewed-By: Chengzhong Wu 
Reviewed-By: Guy Bedford 
Reviewed-By: Geoffrey Booth 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51481
Fixes: https://github.com/nodejs/node/issues/51480
Reviewed-By: Chengzhong Wu 
Reviewed-By: Guy Bedford 
Reviewed-By: Geoffrey Booth 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 16 Jan 2024 00:18:21 GMT
   ✔  Approvals: 3
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/51481#pullrequestreview-1831785070
   ✔  - Guy Bedford (@guybedford): https://github.com/nodejs/node/pull/51481#pullrequestreview-1822522624
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/51481#pullrequestreview-1822572122
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-01-19T07:06:33Z: https://ci.nodejs.org/job/node-test-pull-request/56816/
- Querying data for job/node-test-pull-request/56816/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 51481
From https://github.com/nodejs/node
 * branch                  refs/pull/51481/merge -> FETCH_HEAD
✔  Fetched commits as eb4432c12c67..da6e2ca80b02
--------------------------------------------------------------------------------
[main d234630551] module: fix crash when built-in module export a `default` key
 Author: Antoine du Hamel 
 Date: Tue Jan 16 01:17:29 2024 +0100
 2 files changed, 10 insertions(+), 1 deletion(-)
 create mode 100644 test/parallel/test-process-default.js
[main 3376c26fbc] Update lib/internal/bootstrap/realm.js
 Author: Antoine du Hamel 
 Date: Tue Jan 16 10:35:42 2024 +0100
 1 file changed, 2 insertions(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
module: fix crash when built-in module export a default key

PR-URL: #51481
Fixes: #51480
Reviewed-By: Chengzhong Wu legendecas@gmail.com
Reviewed-By: Guy Bedford guybedford@gmail.com
Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

[detached HEAD a5e1029a22] module: fix crash when built-in module export a default key
Author: Antoine du Hamel duhamelantoine1995@gmail.com
Date: Tue Jan 16 01:17:29 2024 +0100
2 files changed, 10 insertions(+), 1 deletion(-)
create mode 100644 test/parallel/test-process-default.js
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update lib/internal/bootstrap/realm.js

Co-authored-by: Geoffrey Booth webadmin@geoffreybooth.com
PR-URL: #51481
Fixes: #51480
Reviewed-By: Chengzhong Wu legendecas@gmail.com
Reviewed-By: Guy Bedford guybedford@gmail.com
Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

[detached HEAD 7e9f2cd03d] Update lib/internal/bootstrap/realm.js
Author: Antoine du Hamel duhamelantoine1995@gmail.com
Date: Tue Jan 16 10:35:42 2024 +0100
1 file changed, 2 insertions(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/7580926436

@legendecas legendecas added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 19, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2024
@nodejs-github-bot nodejs-github-bot merged commit a772973 into nodejs:main Jan 19, 2024
71 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a772973

@aduh95 aduh95 deleted the process-default branch January 19, 2024 09:04
Medhansh404 pushed a commit to Medhansh404/node that referenced this pull request Jan 19, 2024
PR-URL: nodejs#51481
Fixes: nodejs#51480
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Jan 22, 2024
PR-URL: nodejs#51481
Fixes: nodejs#51480
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 2, 2024
PR-URL: nodejs#51481
Fixes: nodejs#51480
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
targos pushed a commit that referenced this pull request Feb 15, 2024
PR-URL: #51481
Fixes: #51480
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024
PR-URL: nodejs#51481
Fixes: nodejs#51480
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51481
Fixes: #51480
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51481
Fixes: #51480
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crash after set process.default to any value and then firstly require or import process module
7 participants