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

HBSPS · 2025-03-05T02:44:05Z

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

codecov · 2025-03-05T04:12:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (db00f94) to head (6e529a4).
Report is 319 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57324      +/-   ##
==========================================
+ Coverage   90.20%   90.22%   +0.01%     
==========================================
  Files         630      630              
  Lines      185166   185203      +37     
  Branches    36227    36247      +20     
==========================================
+ Hits       167036   167102      +66     
+ Misses      11117    11044      -73     
- Partials     7013     7057      +44

Files with missing lines	Coverage Δ
lib/internal/util.js	`96.05% <100.00%> (+0.24%)`	⬆️
lib/os.js	`98.59% <100.00%> (+1.27%)`	⬆️

... and 63 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 · 2025-03-05T15:32:34Z

Can you please add a test?

HBSPS · 2025-03-07T03:26:43Z

Can you please add a test?

@lpinca This function is only being used by networkInterfaces, which is getting it from internalBinding.

Since networkInterfaces is not a pure function and returns a value based on the current state of the system, is there any way to do mocking or something in this regard?
(I'm not familiar with this kind of testing)

If you have any references or know how to do it, I'd appreciate it :)

function networkInterfaces() {
  const data = getInterfaceAddresses();
  const result = {};

  if (data === undefined)
    return result;
  for (let i = 0; i < data.length; i += 7) {
    const name = data[i];
    const entry = {
      address: data[i + 1],
      netmask: data[i + 2],
      family: data[i + 3],
      mac: data[i + 4],
      internal: data[i + 5],
      cidr: getCIDR(data[i + 1], data[i + 2], data[i + 3]),
    };
    const scopeid = data[i + 6];
    if (scopeid !== -1)
      entry.scopeid = scopeid;

    const existing = result[name];
    if (existing !== undefined)
      ArrayPrototypePush(existing, entry);
    else
      result[name] = [entry];
  }

  return result;
}

lpinca · 2025-03-08T15:43:01Z

What do think about moving it to lib/internal/util.js or lib/internal/os/utils.js? The latter does not exist yet.

move to util/internal and add getCIDR function test file to test the getCIDR function and added test code for the part that checks the format of the subnetmask

HBSPS · 2025-03-09T18:40:14Z

What do think about moving it to lib/internal/util.js or lib/internal/os/utils.js? The latter does not exist yet.

@lpinca Sounds like a good idea, I moved the getCIDR function to internal/util as you said and created a new test file to test that function.

HBSPS · 2025-04-02T23:06:14Z

@lpinca PR seems to have stopped here, what should I do?

nodejs-github-bot · 2025-04-19T21:27:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66371/ 💛

nodejs-github-bot · 2025-04-20T05:39:09Z

Commit Queue failed

- Loading data for nodejs/node/pull/57324
✔  Done loading data for nodejs/node/pull/57324
----------------------------------- PR info ------------------------------------
Title      os: fix netmask format check condition in getCIDR function (#57324)
Author     Wiyeong Seo <[email protected]> (@HBSPS)
Branch     HBSPS:os-getCIDR -> nodejs:main
Labels     os, needs-ci
Commits    2
 - os: fix netmask format check condition in getCIDR function
 - test: move getCIDR function to internal/util and add test code
Committers 1
 - HBSPS <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/57324
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/57324
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 05 Mar 2025 02:44:05 GMT
   ✔  Approvals: 2
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/57324#pullrequestreview-2669497839
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/57324#pullrequestreview-2780071018
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-04-19T21:27:46Z: https://ci.nodejs.org/job/node-test-pull-request/66371/
- Querying data for job/node-test-pull-request/66371/
   ✔  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 57324
From https://github.com/nodejs/node
 * branch                  refs/pull/57324/merge -> FETCH_HEAD
✔  Fetched commits as 25842c5e35ef..6e529a46153e
--------------------------------------------------------------------------------
[main c2a5b7fda5] os: fix netmask format check condition in getCIDR function
 Author: HBSPS <[email protected]>
 Date: Tue Mar 4 20:27:46 2025 -0600
 1 file changed, 2 insertions(+), 1 deletion(-)
Auto-merging lib/internal/util.js
[main dd12864109] test: move getCIDR function to internal/util and add test code
 Author: HBSPS <[email protected]>
 Date: Sun Mar 9 13:36:08 2025 -0500
 3 files changed, 81 insertions(+), 56 deletions(-)
 create mode 100644 test/parallel/test-internal-util-getCIDR.js
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
os: fix netmask format check condition in getCIDR function
Modified to check the format of the netmask instead

of just checking that each part of the netmask is even
PR-URL: #57324

Reviewed-By: Luigi Pinca <[email protected]>

Reviewed-By: James M Snell <[email protected]>
[detached HEAD 6933aa3823] os: fix netmask format check condition in getCIDR function

Author: HBSPS <[email protected]>

Date: Tue Mar 4 20:27:46 2025 -0600

1 file changed, 2 insertions(+), 1 deletion(-)

Rebasing (3/4)

Rebasing (4/4)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

test: move getCIDR function to internal/util and add test code
move to util/internal and add getCIDR function test file

to test the getCIDR function and added test code for the part

that checks the format of the subnetmask
PR-URL: #57324

Reviewed-By: Luigi Pinca <[email protected]>

Reviewed-By: James M Snell <[email protected]>
[detached HEAD a054662fbd] test: move getCIDR function to internal/util and add test code

Author: HBSPS <[email protected]>

Date: Sun Mar 9 13:36:08 2025 -0500

3 files changed, 81 insertions(+), 56 deletions(-)

create mode 100644 test/parallel/test-internal-util-getCIDR.js

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/14556471756

nodejs-github-bot · 2025-04-20T05:54:50Z

Landed in 7102ea1

Modified to check the format of the netmask instead of just checking that each part of the netmask is even PR-URL: #57324 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

os: fix netmask format check condition in getCIDR function

c2e41b8

Modified to check the format of the netmask instead of just checking that each part of the netmask is even

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

lpinca approved these changes Mar 5, 2025

View reviewed changes

test: move getCIDR function to internal/util and add test code

6e529a4

move to util/internal and add getCIDR function test file to test the getCIDR function and added test code for the part that checks the format of the subnetmask

lpinca approved these changes Mar 9, 2025

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 9, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 9, 2025

This comment was marked as outdated.

Sign in to view

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2025

This comment was marked as outdated.

Sign in to view

jasnell approved these changes Apr 19, 2025

View reviewed changes

lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 20, 2025

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 Apr 20, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 20, 2025

nodejs-github-bot merged commit 7102ea1 into nodejs:main Apr 20, 2025
75 checks passed

RafaelGSS mentioned this pull request May 1, 2025

2025-05-06, Version 24.0.0 (Current) #57609

Merged

github-actions bot mentioned this pull request May 19, 2025

2025-05-21, Version 22.16.0 'Jod' (LTS) #58388

Merged

marco-ippolito mentioned this pull request Jun 5, 2025

v20.19.3 proposal #58588

Open

AuntyEms mentioned this pull request Jun 8, 2025

### Notable Changes autofix-ci/action#28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

HBSPS commented Mar 5, 2025

Uh oh!

codecov bot commented Mar 5, 2025 •

edited

Loading

Uh oh!

lpinca commented Mar 5, 2025

Uh oh!

HBSPS commented Mar 7, 2025

Uh oh!

lpinca commented Mar 8, 2025 •

edited

Loading

Uh oh!

HBSPS commented Mar 9, 2025

Uh oh!

This comment was marked as outdated.

HBSPS commented Apr 2, 2025 •

edited

Loading

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Apr 19, 2025 •

edited by jasnell

Loading

Uh oh!

nodejs-github-bot commented Apr 20, 2025

PR-URL: #57324
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

PR-URL: #57324
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 20, 2025

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

HBSPS commented Mar 5, 2025

Uh oh!

codecov bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lpinca commented Mar 5, 2025

Uh oh!

HBSPS commented Mar 7, 2025

Uh oh!

lpinca commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HBSPS commented Mar 9, 2025

Uh oh!

This comment was marked as outdated.

HBSPS commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Apr 19, 2025 • edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 20, 2025

PR-URL: #57324 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #57324 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 20, 2025

Uh oh!

Uh oh!

codecov bot commented Mar 5, 2025 •

edited

Loading

lpinca commented Mar 8, 2025 •

edited

Loading

HBSPS commented Apr 2, 2025 •

edited

Loading

nodejs-github-bot commented Apr 19, 2025 •

edited by jasnell

Loading

PR-URL: #57324
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

PR-URL: #57324
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>