[Node.js 기여하기] 4. os.networkInterfaces의 서브넷마스크 조건 수정하기

https://github.com/nodejs/node/pull/57324

os: fix netmask format check condition in getCIDR function by HBSPS · Pull Request #57324 · nodejs/node

 

이번에는 성능 개선이 아니라 Node.js에서 올바르지 않게 동작하던 부분을 수정할 수 있었다.

 

수정 전 코드는 아래와 같다.

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;
}

function getCIDR(address, netmask, family) {
  let ones = 0;
  let split = '.';
  let range = 10;
  let groupLength = 8;
  let hasZeros = false;
  let lastPos = 0;

  if (family === 'IPv6') {
    split = ':';
    range = 16;
    groupLength = 16;
  }

  for (let i = 0; i < netmask.length; i++) {
    if (netmask[i] !== split) {
      if (i + 1 < netmask.length) {
        continue;
      }
      i++;
    }
    const part = StringPrototypeSlice(netmask, lastPos, i);
    lastPos = i + 1;
    if (part !== '') {
      if (hasZeros) {
        if (part !== '0') {
          return null;
        }
      } else {
        const binary = NumberParseInt(part, range);
        const binaryOnes = countBinaryOnes(binary);
        ones += binaryOnes;
        if (binaryOnes !== groupLength) {
          if ((binary & 1) !== 0) {
            return null;
          }
          hasZeros = true;
        }
      }
    }
  }

  return `${address}/${ones}`;
}

https://nodejs.org/docs/latest/api/os.html#osnetworkinterfaces

OS | Node.js v23.11.0 Documentation

위 코드의 경우 OS 모듈에 포함된 메소드 중 하나로 실행 결과는 아래와 같다.

{
  lo: [
    {
      address: '127.0.0.1',
      netmask: '255.0.0.0',
      family: 'IPv4',
      mac: '00:00:00:00:00:00',
      internal: true,
      cidr: '127.0.0.1/8'
    },
    {
      address: '::1',
      netmask: 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff',
      family: 'IPv6',
      mac: '00:00:00:00:00:00',
      scopeid: 0,
      internal: true,
      cidr: '::1/128'
    }
  ],
  eth0: [
    {
      address: '192.168.1.108',
      netmask: '255.255.255.0',
      family: 'IPv4',
      mac: '01:02:03:0a:0b:0c',
      internal: false,
      cidr: '192.168.1.108/24'
    },
    {
      address: 'fe80::a00:27ff:fe4e:66a1',
      netmask: 'ffff:ffff:ffff:ffff::',
      family: 'IPv6',
      mac: '01:02:03:0a:0b:0c',
      scopeid: 1,
      internal: false,
      cidr: 'fe80::a00:27ff:fe4e:66a1/64'
    }
  ]
}

 

위의 출력 예시에서 볼 수 있듯이 단순히 현재 시스템의 네트워크 정보를 가져오는 함수다.

 

개선점 찾기

위 함수는 인자가 없으며 내부적으로 시스템의 정보를 가져와 반환하는 것으로 보인다.

 

단순히 전체 네트워크 정보들을 순회하며 정해진 데이터를 만들어 반환하는 것이다.

 

사실 저 부분에서는 별도로 개선할 점이 없어보인다.

 

문제는 해당 함수 내부에서 사용하는 getCIDR 함수다.

 

이 함수의 경우 당시까지 networkInterfaces 메소드 내부에서만 사용하고 있었으며 CIDR을 생성해 반환하는 함수다.

 

세 개의 데이터를 인자로 받으며 각각 IP, 서브넷마스크, family (IPv4인지 IPv6인지)로 구성되어 있다.

 

이 함수의 최종 반환 결과는 127.0.0.1/8과 같이 CIDR 형식의 문자열이다.

 

이를 생각하며 다시 이 코드를 읽어본다면 이 함수가 무슨 역할을 하는지 쉽게 알 수 있을 것이다.

function getCIDR(address, netmask, family) {
  let ones = 0;
  let split = '.';
  let range = 10;
  let groupLength = 8;
  let hasZeros = false;
  let lastPos = 0;

  if (family === 'IPv6') {
    split = ':';
    range = 16;
    groupLength = 16;
  }

  for (let i = 0; i < netmask.length; i++) {
    if (netmask[i] !== split) {
      if (i + 1 < netmask.length) {
        continue;
      }
      i++;
    }
    const part = StringPrototypeSlice(netmask, lastPos, i);
    lastPos = i + 1;
    if (part !== '') {
      if (hasZeros) {
        if (part !== '0') {
          return null;
        }
      } else {
        const binary = NumberParseInt(part, range);
        const binaryOnes = countBinaryOnes(binary);
        ones += binaryOnes;
        if (binaryOnes !== groupLength) {
          if ((binary & 1) !== 0) {
            return null;
          }
          hasZeros = true;
        }
      }
    }
  }

  return `${address}/${ones}`;
}

 

여기서 문제가 되는 부분은 다음과 같다.

// 생략

if (part !== '') {
  if (hasZeros) {
    if (part !== '0') {
      return null;
    }
  } else {
    const binary = NumberParseInt(part, range);
    const binaryOnes = countBinaryOnes(binary);
    ones += binaryOnes;
    if (binaryOnes !== groupLength) {
      if ((binary & 1) !== 0) {
        return null;
      }
      hasZeros = true;
    }
  }
}

// 생략

 

예를 들어 아래와 같은 경우가 있다고 해보자.

• ip: 127.0.0.1
• netmask: 255.242.0.0
• family: IPv4

 

위의 예시대로 코드가 실행된다면 아래와 같은 결과가 반환될 것이다.

1. (split에 해당하는 .을 만날 때 까지 netmask를 한 자리씩 읽기 순서로 탐색. 최종적으로 part는 255가 된 상태에서 if문 진입)
2. part가 비어있지 않으므로 (255) else문 실행
3. binary = 255 (int) / binaryOnes = 8 / ones = 0 + 8
4. binaryOnes와 groupLength가 8로 동일하므로 이후 코드는 실행되지 않음
5. 두 번째 part는 242이므로 else문 실행
6. binary = 242 (int) / binaryOnes = 5 (11110010) / ones = 8 + 5
7. binaryOnes와 groupLength가 8로 동일하지 않으므로 if문 실행
8. (binary & 1 = 0)이므로 조건문이 참이 됨
9. hasZero가 true로 변경
10. 이후 두 번의 반복에 대해 netmask가 비어있지 않고 hasZero가 true며 각 part가 0이기 때문에 null이 반환되지 않음
11. 최종 결과는 127.0.0.1/13

하지만 본인의 경우 학교 수업을 들으며 서브넷마스크의 경우 연속된 1로 구성되어 있으며 한 번 0이 나온 뒤에는 다시 1이 나와선 안된다고 배웠다.

 

또한 다양한 RFC 문서에서 이를 언급하고 있다.

https://datatracker.ietf.org/doc/html/rfc1519#section-4

RFC 1519: Classless Inter-Domain Routing (CIDR): an Address Assignment and Aggregation Strategy

https://datatracker.ietf.org/doc/html/rfc4632#section-5.1

RFC 4632: Classless Inter-domain Routing (CIDR): The Internet Address Assignment and Aggregation Plan

An implementation following these rules should also be generalized,  so that an arbitrary network number and mask are accepted for all routing destinations.  The only outstanding constraint is that the mask must be left contiguous.

이러한 규칙을 따르는 구현도 일반화하여 모든 라우팅 대상에 대해 임의의 네트워크 번호와 마스크가 허용되도록 해야 합니다.  유일한 제약 조건은 마스크가 연속적으로 유지되어야 한다는 것입니다.

 

개선 방법

현재의 조건문은 단순히 해당 숫자가 짝수인지 확인만 하고 있다.

 

만약 연속된 1이 아닌 서브넷 마스크가 허용된다고 하더라도 단순히 짝수인지 검사하는 조건은 필요 없을 것이다.

 

왜냐하면 이러한 경우 모든 형태의 서브넷 마스크가 허용되므로 홀수 서브넷 마스크만 허용되지 않을 이유가 없다.

 

그렇다면 두 가지 방법이 있다.

1. 조건문 자체를 없애기
2. 조건문을 위의 조건에 맞도록 수정하기

우선 내가 배웠던 내용과 함께 RFC 문서라는 근거가 뒷받침 되기 때문에 2번을 선택했다.

 

서브넷 마스크가 유효한지 찾기 위해 선택한 방법은 아래와 같다.

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

단순히 숫자를 2진수 문자열로 변환한 뒤 문자열 01이 포함되는지 확인하는 것이다.

 

이는 한 번 0이 나온 뒤 다시 1이 나올 수 없다는 서브넷 마스크의 조건을 만족하는지 검사하는 가장 쉬운 방법이라 생각했다.

 

1차 리뷰 - 테스트 파일 추가

이때까지의 기여와는 다르게 벤치마크 결과도 없으며 이 변경이 승인되기 위해서는 기술적으로 설득할 수 있어야 한다고 생각했다.

 

또한, 위에서 언급한 두 가지 방법을 모두 제시한 뒤 의견이 다른 경우 그 의견을 따르고자 했다.

 

첫 번째 승인을 받는데 큰 어려움은 없었으나 새로운 변경을 요구받았다.

 

실제로 기존 getCIDR의 문제가 되는 조건문은 테스트 커버리지에 포함되지 않고 있었다.

(이를 테스트 하는 파일이 없었다)

 

문제점 - 테스트 파일을 작성할 수 없음

이 함수는 networkInterfaces 내부에서만 사용되고 있었으며 networkInterfaces의 경우 인자가 없고 시스템 정보를 활용하기 때문에 테스트 파일을 작성하는데 어려움이 있었다.

 

가장 먼저 생각한 부분은 mocking이었으나 시스템 내부의 정보를 가져오는 것은 internal 부분에 포함된 코드인 것으로 보이기 때문에 이것을 mocking 하는데 어려움이 있을 것으로 보였다.

 

두 번째 방법으로 getCIDR 함수의 경우 순수함수기 때문에 이 함수 자체를 바로 테스트 할 수 있는 방법을 찾고자 했다.

 

하지만 이 파일의 경우도 networkInterfaces와 같은 파일에 작성되어 있었으며 내보내고 있지 않았기 때문에 require를 통해 불러올 수 없었다.

 

해결 - 함수를 다른 파일로 이동

이를 해결하기 위해 우선 첫 번째 리뷰를 해주신 분께 도움을 요청드렸다.

 

물론 두 번째 방법이 조금 더 합리적이라는 것을 알고는 있으나 기존의 코드를 다른 파일로 이동하는 것과 새로운 파일을 생성하는 것에 대해 확실한 답변을 받는 것이 나을 것으로 생각했다.

 

답변에 따르면 새로운 파일을 생성하여 이동하는 것이 가능했다.

 

따라서 기존에 있던 getCIDR 함수와 내부에서만 사용하는 다양한 함수들을 internal/util.js로 이동했다.

 

internal 내부에 있는 함수들은 내보내기를 해도 사용자가 일반적인 방법으로 require 할 수 없도록 되어있다. (말 그대로 내부에서 사용하기 위한 함수들의 모음)

 

내부적으로는 이 함수를 require하는데 문제가 없다.

 

테스트 파일의 경우는 일반적인 테스트 파일과 동일하게 작성할 수 있었다.

// Flags: --expose-internals
'use strict';
require('../common');

// These are tests that verify that the subnetmask is used
// to create the correct CIDR address.
// Tests that it returns null if the subnetmask is not in the correct format.
// (ref: https://www.rfc-editor.org/rfc/rfc1878)

const assert = require('node:assert');
const { getCIDR } = require('internal/util');

assert.strictEqual(getCIDR('127.0.0.1', '255.0.0.0', 'IPv4'), '127.0.0.1/8');
assert.strictEqual(getCIDR('127.0.0.1', '255.255.0.0', 'IPv4'), '127.0.0.1/16');

// 242 = 11110010(2)
assert.strictEqual(getCIDR('127.0.0.1', '242.0.0.0', 'IPv4'), null);

assert.strictEqual(getCIDR('::1', 'ffff:ffff:ffff:ffff::', 'IPv6'), '::1/64');
assert.strictEqual(getCIDR('::1', 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', 'IPv6'), '::1/128');

// ff00:ffff = 11111111 00000000 : 11111111 11111111(2)
assert.strictEqual(getCIDR('::1', 'ffff:ff00:ffff::', 'IPv6'), null);

 

결과

리뷰까지 시간이 오래 걸리기는 했으나 성공적으로 승인을 받을 수 있었다.

 

테스트 파일도 작성해볼 수 있었고 모르는 부분에 대해 도움도 받을 수 있는 PR이라 생각한다.

 

모르는 것이 있거나 그들의 컨벤션을 잘 따르기 위해서 질문을 한다면 보다 수준이 높은 기여를 할 수 있을 것이라 생각한다.

 

여담

Node.js의 내부에는 다양한 내부 모듈들이 있으며 이들의 목표는 내부적으로 활용하기 위한 모듈들을 담는 것이다.

 

일반 사용자는 이 모듈을 사용할 일이 없으며 내부에 있는 모듈들은 위와 같이 언제든 바뀔 수 있기 때문에 이를 직접 사용하지 않는 것을 추천한다.

https://github.com/nodejs/node/blob/main/lib/internal/README.md

node/lib/internal/README.md at main · nodejs/node

 

그럼에도 문서를 보면 내부 모듈을 사용할 수 있는 방법이 있긴 하다.

 

node 명령어에 --expose-internal을 사용한다면 내부 모듈을 사용할 수는 있다.

(호기심 많은 사람이라면 직접 사용해볼 수 있다. 단, 실제 프로젝트가 저 부분에 의존해서는 안된다)