net: make multiple improvements to net.BlockList - #64974
Conversation
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Take O(1) fast-path when possible Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode/Claude
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode/Opus
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: OpenCode/Opus
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64974 +/- ##
==========================================
- Coverage 90.27% 90.23% -0.05%
==========================================
Files 762 762
Lines 247515 247941 +426
Branches 46683 46793 +110
==========================================
+ Hits 223447 223730 +283
- Misses 15485 15601 +116
- Partials 8583 8610 +27
🚀 New features to boost your workflow:
|
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
390b7fa to
5623bfb
Compare
| } | ||
|
|
||
| template <typename Callback> | ||
| void SocketAddressBlockList::SubnetTrie::Walk(Callback cb) const { |
There was a problem hiding this comment.
Both Walk and WalkImpl are dead code, these aren't used anywhere.
| if (address.family() == AF_INET) { | ||
| if (ipv4_subnets_.Lookup(bytes, bits)) return true; | ||
| // Also check IPv6 trie for ::ffff:x.x.x.x subnets. | ||
| uint8_t mapped[16] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff}; |
There was a problem hiding this comment.
We're mirroring IPv4 & IPv6 handling in both directions, and we could save some work by just doing one. We duplicate writes (changing anything IPv4-related writes to both ranges) and we duplicate reads (all checks read both ranges). Surely there's a way we can just duplicate the write side and only check once read-side?
| if (prefix >= 96 && memcmp(bytes, v4mapped, 12) == 0) { | ||
| ipv4_subnets_.Remove(bytes + 12, prefix - 96); | ||
| } | ||
| } |
There was a problem hiding this comment.
The removal logic ignores overlaps, which results in a selection of fun ways to get this state out of sync. One example:
bl.addSubnet('10.0.0.0', 8);
bl.addSubnet('10.1.0.0', 16);
bl.removeSubnet('10.0.0.0', 8);
bl.rules // Lists 10.1.0.0./16
bl.check('10.1.2.3') // False - not blockedThis is a bit tricky to fix. If you have rule A & B that overlap, and you remove rule A, you can't clear all of A's range from the trie - you have to clear only the part that didn't overlap. Or you clear everything, but then re-apply B? Fun.
| // Fast path: pass the string directly to C++ which does | ||
| // inet_pton + Apply() without allocating a JS SocketAddress wrapper. | ||
| const af = family === 'ipv4' ? AF_INET : AF_INET6; | ||
| return this[kHandle].checkString(address, af); |
There was a problem hiding this comment.
This no longer does normalizes family which might catch some people out:
> bl.addAddress('10.0.0.1')
> bl.check('10.0.0.1', 'IPv4')
false
> bl.check('10.0.0.1', 'ipv4')
trueBoth return true on main.
| function testFastCheck() { | ||
| assert(blockList.check('1.1.1.1')); | ||
| assert(!blockList.check('2.2.2.2')); | ||
| assert(blockList.check('10.0.0.5')); |
There was a problem hiding this comment.
I think this is supposed to be testing SocketAddressBlockListWrap::FastCheck, is that right? It looks like it doesn't actually test that, because that path is only for SocketAddress args and this all passes strings.
| } | ||
| for (let i = 0; i < cidrs.length; i++) { | ||
| validateString(cidrs[i], `cidrs[${i}]`); | ||
| this.addCIDR(cidrs[i]); |
There was a problem hiding this comment.
It would be nice to validate all in advance, and then apply them all, so that if an exception is thrown then nothing is applied instead of half-applying the array.
New APIs:
addCIDR(cidr): Parse CIDR string, auto-detect familyaddCIDRs(cidrs): Batch CIDR addaddAddresses(addresses): Batch address addremoveAddress(address): Remove exact address ruleremoveRange(start, end): Remove range ruleremoveSubnet(net, prefix): Remove subnet rule (+ trie cleanup)removeCIDR(cidr): Remove subnet by CIDR stringclear(): Remove all rulessize: Rule count without allocating rules arrayBlockList.PRIVATE_RANGES: Array of RFC 1918 + loopback + link-local + ULA CIDRsPerformance improvements:
Adds a benchmark and more tests.
Essentially went from O(n) to mostly O(1) for most cases.
Signed-off-by: James M Snell jasnell@gmail.com