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

cluster, net: fix listen pipe with readable and writable in cluster #43634

Merged

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Jun 30, 2022

fix listen pipe with readable and writable in cluster.

The bug can be triggered by code as follow.

const cluster = require('cluster');
const net = require('net');
const { unlinkSync } = require('fs');

if (cluster.isPrimary) {
    cluster.fork();
} else {
    try {
        unlinkSync('test.sock');
    } catch(e) {}
    net.createServer(() => {}).listen({
        path: 'test.sock',
        readableAll: true,
        writableAll: true,
    });
}

The permissions of test.sock is srwxr-xr-x instead of srwxrwxrwx .

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

Affected subsystem: cluster, net

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Jun 30, 2022
@theanarkh theanarkh force-pushed the fix_pipe_read_write_in_cluster branch from 807b763 to 09ab407 Compare July 1, 2022 00:09
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

You should also update the API docs.
The rest looks fine to me!

@theanarkh
Copy link
Contributor Author

theanarkh commented Jul 1, 2022

You should also update the API docs. The rest looks fine to me!

I think it has been described in the docs before ?

readableAll: For IPC servers makes the pipe readable for all users. Default: false
writableAll: For IPC servers makes the pipe writable for all users. Default: false.

And i think it is a bug, not feature.

@ShogunPanda
Copy link
Contributor

Sorry, I missed those somehow.
Approved, LGTM!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@theanarkh theanarkh force-pushed the fix_pipe_read_write_in_cluster branch from 09ab407 to 14588b3 Compare July 4, 2022 23:14
@theanarkh
Copy link
Contributor Author

@mcollina @ShogunPanda @lpinca Hi, can you help trigger CI ? Thanks !

@ShogunPanda
Copy link
Contributor

@theanarkh The CI is currently locked down due to upcoming security release.
You will have to wait couple of days!

@theanarkh
Copy link
Contributor Author

@theanarkh The CI is currently locked down due to upcoming security release. You will have to wait couple of days!

Thanks! can you help trigger CI ?

@ShogunPanda
Copy link
Contributor

Done sir!

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@theanarkh
Copy link
Contributor Author

@aduh95 Hi, can you help merge this PR ? Thanks !

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 10, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2022
@nodejs-github-bot nodejs-github-bot merged commit a933a75 into nodejs:main Jul 10, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in a933a75

targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43634
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 20, 2022
PR-URL: #43634
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43634
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43634
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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. cluster Issues and PRs related to the cluster subsystem. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants