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.

Sign up for GitHub

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

Jump to bottom

test: check net.ipv4.ip_unprivileged_port_start in parallel/test-cluster-bind-privileged-port #45838

Closed
bnoordhuis opened this issue Dec 13, 2022 · 13 comments · Fixed by #45904 or #46536
Closed

test: check net.ipv4.ip_unprivileged_port_start in parallel/test-cluster-bind-privileged-port #45838

bnoordhuis opened this issue Dec 13, 2022 · 13 comments · Fixed by #45904 or #46536
Labels
good first issue Issues that are suitable for first-time contributors. linux Issues and PRs related to the Linux platform. test Issues and PRs related to the tests.

Comments

@bnoordhuis
Copy link
Member

test/parallel/test-cluster-bind-privileged-port.js expects binding to port 42 to fail but that's not an always-correct assumption.

Check the output of sysctl net.ipv4.ip_unprivileged_port_start because it's possible (albeit unlikely) for that port to be unprivileged.

Example output:

$ sysctl net.ipv4.ip_unprivileged_port_start
net.ipv4.ip_unprivileged_port_start = 1
@bnoordhuis bnoordhuis added test Issues and PRs related to the tests. good first issue Issues that are suitable for first-time contributors. linux Issues and PRs related to the Linux platform. labels Dec 13, 2022
@7suyash7
Copy link
Contributor 

$ sysctl net.ipv4.ip_unprivileged_port_start
net.ipv4.ip_unprivileged_port_start = 1024

Here's the output for me on Ubuntu 22.04.1

@bnoordhuis
Copy link
Member Author

Yes, that's the default (and traditional UNIX behavior.) The test starts failing when it's <= 42.

@7suyash7
Copy link
Contributor

So, you're saying the test should be modified to check the range of reserved ports on the current system, rather than assuming that it will always be 42?

@bnoordhuis
Copy link
Member Author

The test should bail out with common.skip('...') if port 42 is unprivileged, yes.

@7suyash7
Copy link
Contributor

Can I take a try at this? I would write a function to find the first unprivileged port and then try to attach a process to the port just before the first unprivileged port. Then basically the same checks as before (making sure the exit code is 0, etc) Is this implementation fine?
@bnoordhuis

@bnoordhuis
Copy link
Member Author

Go for it but I'd rather it just exits if port 42 is unprivileged. Principle of least surprise and all that.

@7suyash7
Copy link
Contributor

That makes sense, I'll send in a PR in some time!

7suyash7 added a commit to 7suyash7/node that referenced this issue Dec 16, 2022
@7suyash7
test: add check to exit if port 42 is unprivileged
11d4d4a 
Fixes: nodejs#45838
7suyash7 added a commit to 7suyash7/node that referenced this issue Dec 16, 2022
@7suyash7
test: add fix so that test exits if port 42 is unprivileged
83b02d8 
fixes nodejs#45838
@7suyash7
Copy link
Contributor

@bnoordhuis PTAL.

@7suyash7
Copy link
Contributor

Please ignore the second PR, I think it got added due to me changing branches locally and pushing.

@bnoordhuis
Copy link
Member Author

Can you link me to your PR? I can't find it for some reason.

@7suyash7 7suyash7 mentioned this issue Dec 18, 2022
Merged
@7suyash7
Copy link
Contributor

@bnoordhuis I messed up my Git locally and had to create a new PR. I hope you can see it now?

nodejs-github-bot pushed a commit that referenced this issue Jan 18, 2023
@7suyash7
test: add fix so that test exits if port 42 is unprivileged
60cc1ba 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this issue Jan 20, 2023
@7suyash7 @RafaelGSS
test: add fix so that test exits if port 42 is unprivileged
4f491d3 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
juanarbol pushed a commit that referenced this issue Jan 26, 2023
@7suyash7 @juanarbol
test: add fix so that test exits if port 42 is unprivileged
c1f2c8d 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
juanarbol pushed a commit that referenced this issue Jan 31, 2023
@7suyash7 @juanarbol
test: add fix so that test exits if port 42 is unprivileged
896027c 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@KrayzeeKev
Copy link
Contributor

This "fix" has meant that the test suite can not run on Linux kernels < 4.1 because that sysctl feature does not exist.
I'm building for CentOS 7 which is kernel 3.10 - And the reason I'm building is that the official build moved to RHEL 8. NodeJS itself does not need this kernel feature, only the guard case in the test.
I'm going to work on a fix for this that assumed that the priv port limit is 1024 if that sysctl fails (which seems like a reasonable presumption)

@KrayzeeKev KrayzeeKev mentioned this issue Feb 7, 2023
Merged
@KrayzeeKev
Copy link
Contributor

I've put in the above PR #46536 - to fix v18 as well, do I just do another branch with a -t upstream/v18.x ?

@KrayzeeKev KrayzeeKev mentioned this issue Feb 7, 2023
Closed
nodejs-github-bot pushed a commit that referenced this issue Mar 2, 2023
@KrayzeeKev
test: assume priv ports start at 1024 if it can't be changed
024e648 
An update to test/parallel/test-cluster-bind-privileged-port.js
checks the lowest privileged port to ensure 42 is privileged
This only works on kernels > 4.1. On older kernels, this is
locked at 1024 so the check is not needed.

Fixes: #45838
PR-URL: #46536
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Mar 13, 2023
@KrayzeeKev @targos
test: assume priv ports start at 1024 if it can't be changed
e509018 
An update to test/parallel/test-cluster-bind-privileged-port.js
checks the lowest privileged port to ensure 42 is privileged
This only works on kernels > 4.1. On older kernels, this is
locked at 1024 so the check is not needed.

Fixes: #45838
PR-URL: #46536
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Mar 14, 2023
@KrayzeeKev @targos
test: assume priv ports start at 1024 if it can't be changed
db2ace1 
An update to test/parallel/test-cluster-bind-privileged-port.js
checks the lowest privileged port to ensure 42 is privileged
This only works on kernels > 4.1. On older kernels, this is
locked at 1024 so the check is not needed.

Fixes: #45838
PR-URL: #46536
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 11, 2023
@KrayzeeKev @danielleadams
test: assume priv ports start at 1024 if it can't be changed
3fdcf7b 
An update to test/parallel/test-cluster-bind-privileged-port.js
checks the lowest privileged port to ensure 42 is privileged
This only works on kernels > 4.1. On older kernels, this is
locked at 1024 so the check is not needed.

Fixes: #45838
PR-URL: #46536
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. linux Issues and PRs related to the Linux platform. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
3 participants
@bnoordhuis @KrayzeeKev @7suyash7