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

os: fix netmask format check condition in getCIDR function #57324

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HBSPS
Copy link
Contributor

@HBSPS HBSPS commented Mar 5, 2025

Modified to check the format of the netmask instead of just checking that each part of the netmask is even number.
(Ref: https://www.rfc-editor.org/rfc/rfc1878)

The result of the previous run was like this.

...
if ((binary & 1) !== 0) {
  return null;
}
...

console.log(getCIDR('127.0.0.1', '242.0.0.0', 'IPv4')); // 127.0.0.1/5
// 242 = 11110010(2)

This does not satisfy the condition in netmask, but it passes the conditional statement and returns an invalid value.

I'm not sure if that conditional is necessary(if the correct value is always passed, etc), but I think it's doing the wrong thing, so I've modified it to look like this.

...
if (StringPrototypeIncludes(binary.toString(2), '01')) {
  return null;
}

console.log(getCIDR('127.0.0.1', '242.0.0.0', 'IPv4')); // null
...

I modified the netmask to check if it satisfies the condition of containing '01', such as 11110010, since a 0 cannot be followed by a 1 according to the rules of netmask.

The benchmark results for changing conditions are shown below.

                                confidence improvement accuracy (*)   (**)  (***)
os/networkInterfaces.js n=10000                 0.19 %       ±1.17% ±1.56% ±2.03%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 1 comparisons, you can thus expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

Modified to check the format of the netmask instead
of just checking that each part of the netmask is even
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem. labels Mar 5, 2025
Copy link

codecov bot commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.21%. Comparing base (db00f94) to head (c2e41b8).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
lib/os.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57324      +/-   ##
==========================================
+ Coverage   90.20%   90.21%   +0.01%     
==========================================
  Files         630      630              
  Lines      185166   185167       +1     
  Branches    36227    36224       -3     
==========================================
+ Hits       167036   167057      +21     
+ Misses      11117    11109       -8     
+ Partials     7013     7001      -12     
Files with missing lines Coverage Δ
lib/os.js 97.32% <50.00%> (+<0.01%) ⬆️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lpinca
Copy link
Member

lpinca commented Mar 5, 2025

Can you please add a test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants