Skip to content

Loading…

net: More accurate IP address validation and IPv6 dotted notation. #4103

Closed
wants to merge 1 commit into from

6 participants

@snoj

Added isIP method to make use of inet_pton to cares_wrap.cc
Modified net.isIP() to make use of new c++ isIP method.
Added new tests to test-net-isip.js.

@snoj snoj net: More accurate IP address validation and IPv6 dotted notation.
Added isIP method to make use of inet_pton to cares_wrap.cc
Modified net.isIP() to make use of new c++ isIP method.
Added new tests to test-net-isip.js.
1227174
@tjfontaine

I wonder if this shouldn't come along with a benchmark

@bnoordhuis
Node.js Foundation member

I kind of doubt that isIP() is a significant bottleneck in any way. Most of my professional life consists of looking at profiler output and I tell you isIP() never shows up. The answer is no unless you can present some convincing numbers. :-)

@snoj

Sorry it's not pretty, but here's the code I used to test my modifications. http://pastebin.com/5PZgbKWe Tests on each IPv6 address are ran 10,000 times.

It's not always faster and sometimes slower, but I feel since this change is far more accurate it is worthwhile.

//output
old net.isIP :a:b:c:: 9ms
new net.isIP :a:b:c:: 7ms
old net.isIP a:b:c: 6ms
new net.isIP a:b:c: 7ms
old net.isIP a:b:c:: 5ms
new net.isIP a:b:c:: 8ms
old net.isIP :a:b:c: 5ms
new net.isIP :a:b:c: 5ms
old net.isIP :2001:252:0:1::2008:6:: 4ms
new net.isIP :2001:252:0:1::2008:6:: 6ms
old net.isIP 0000:0000:0000:0000:0000:0000:0000:0000: 6ms
new net.isIP 0000:0000:0000:0000:0000:0000:0000:0000: 18ms
old net.isIP 0000:0000:0000:0000:0000:0000:0000:0000::0000: 11ms
new net.isIP 0000:0000:0000:0000:0000:0000:0000:0000::0000: 19ms
old net.isIP ::t: 5ms
new net.isIP ::t: 6ms
old net.isIP ::: 4ms
new net.isIP ::: 7ms
old net.isIP a::b::c: 5ms
new net.isIP a::b::c: 8ms
old net.isIP ::122:1:1: 4ms
new net.isIP ::122:1:1: 8ms
old net.isIP 1::: 4ms
new net.isIP 1::: 7ms
old net.isIP ::a11:abcd: 5ms
new net.isIP ::a11:abcd: 8ms
old net.isIP 2001:aaa::41: 5ms
new net.isIP 2001:aaa::41: 9ms
@bnoordhuis
Node.js Foundation member

I like that it's more correct but I'm still leaning towards no. Can't this be done in pure JS? The only reason the current address parser is so lackadaisical is because a) no one got around to fixing it yet, and b) no one was bothered enough to file a bug report.

@piscisaureus

I am +1 on this. I actually prefer this approach over a pure-js solution.

@snoj

I had considered doing it in pure JS (https://groups.google.com/forum/?fromgroups=#!topic/nodejs/xI4wO9eI-po) but then after some testing had decided against it.

The first was my attempts to do so at first resulted in greater than 100ms times (3-400 sometimes). The code did use split, so that may have been the culprit. I had also thought to port the relevant portions of inet_pton to JS (or find someone who had), but after seeing the code for it, I figured it would be easier and faster to just use what's already there.

@bnoordhuis
Node.js Foundation member

I am +1 on this. I actually prefer this approach over a pure-js solution.

Why? Parsing addresses is not rocket science. I'd rather reduce the body of code I have to worry about. If it lives in lib/, I can make it Isaac's or Nathan's problem. :-)

@piscisaureus

Why? Parsing addresses is not rocket science. I'd rather reduce the body of code I have to worry about. If it lives in lib/, I can make it Isaac's or Nathan's problem. :-)

We already have a parser, it lives in libuv. Creating another in JS means that we have to be sure that they both do exactly the same (if we care about correctness).

Writing an IPv4 parser in js is very simple, but I think writing a correct IPv6 parser in javascript gives us more code to worry about than maintaining a very simple binding.

@snoj

If anyone cares, inet_pton6 in JS. Testing the invalid "0000:0000:0000:0000:0000:0000:0000:0000::0000" 10,000 times comes in at a whopping 175ms. There could issues with my port or things that could be done better to speed it up.

@bnoordhuis
Node.js Foundation member

@snoj You're doing an .indexOf() in your inner loop. This patch speeds it up for me by 50%.

diff --git a/inet_pton.js b/inet_pton.js
index 8df4330..7963c8d 100644
--- a/inet_pton.js
+++ b/inet_pton.js
@@ -35,9 +35,8 @@

  function inet_pton6 (src)
 {
-  var xdigits, tmp, tp, endp, colonp, curtok, ch, saw_xdigit, val;
+  var tmp, tp, endp, colonp, curtok, ch, saw_xdigit, val;

-  xdigits = "0123456789abcdef";
   tmp = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0];

   colonp = null;
@@ -53,10 +52,7 @@
   for(var i = 0; i<src.length; i++)
     { 
       ch = parseInt(src[i],16) //src.charCodeAt(i)
-      var pch;
-
-      pch = xdigits.indexOf(src[i]);
-      if (pch != -1)
+      if (!isNaN(ch))
         { 
           val <<= 4;
           val |= (ch);

One can probably shave off more cycles but this was the most obvious low-hanging fruit.

@snoj

@bnoordhuis, nice!

I am in @piscisaureus' corner though. Two IP address parsers that need to give the same results 100% would be a PITA to maintain.

I can see some valid reasons for having a pure JS version though. Code could be easily brought over to be used in a browser instead of being just a node feature. However, if that's the aim of all the /libs, I don't see how that could easily happen. For instance, DNS (where net.isIP is eventually supposed to go?) is basically all in cares_wrap.cc anyway.

@snoj

O Joyent Overlords. Should I look to a JS only parser for IP validation or shall I wait for a future version of Node.

Amen

@bnoordhuis
Node.js Foundation member

O Joyent Overlords. Should I look to a JS only parser for IP validation or shall I wait for a future version of Node.

Actually, out of the four full-time maintainers, only one works at Joyent. The proper invocation is: "Oh Joyent, Cloud9 and Nodejitsu Overlords". :-)

I still think a pure JS is better because in a perfect world, anything that doesn't do I/O is pure JS. That said, a wrapper around the C functions is a lot easier to implement. I'll throw in my lot with @piscisaureus for now.

@TooTallNate

Don't forget LearnBoost!

@snoj

A thousand apologies my overlords!

@piscisaureus

I'd rather be addressed as "your excellence" rather than "overlord". @snoj

@bnoordhuis
Node.js Foundation member

I'm a simple guy, I'll settle for "lord and master". Sorry, forgot to merge the actual patch. Landed in fb6377e. Thanks, Josh!

@bnoordhuis bnoordhuis closed this
@isaacs

Awesome, thanks guys.

-- Joyent Overlord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 9, 2012
  1. @snoj

    net: More accurate IP address validation and IPv6 dotted notation.

    snoj committed
    Added isIP method to make use of inet_pton to cares_wrap.cc
    Modified net.isIP() to make use of new c++ isIP method.
    Added new tests to test-net-isip.js.
Showing with 27 additions and 21 deletions.
  1. +2 −19 lib/net.js
  2. +15 −1 src/cares_wrap.cc
  3. +10 −1 test/simple/test-net-isip.js
View
21 lib/net.js
@@ -24,6 +24,7 @@ var Stream = require('stream');
var timers = require('timers');
var util = require('util');
var assert = require('assert');
+var cares = process.binding('cares_wrap');
var cluster;
function noop() {}
@@ -1114,26 +1115,8 @@ Server.prototype.unref = function() {
// TODO: isIP should be moved to the DNS code. Putting it here now because
// this is what the legacy system did.
-// NOTE: This does not accept IPv6 with an IPv4 dotted address at the end,
-// and it does not detect more than one double : in a string.
exports.isIP = function(input) {
- if (!input) {
- return 0;
- } else if (/^(\d?\d?\d)\.(\d?\d?\d)\.(\d?\d?\d)\.(\d?\d?\d)$/.test(input)) {
- var parts = input.split('.');
- for (var i = 0; i < parts.length; i++) {
- var part = parseInt(parts[i]);
- if (part < 0 || 255 < part) {
- return 0;
- }
- }
- return 4;
- } else if (/^::|^::1|^([a-fA-F0-9]{1,4}::?){1,7}([a-fA-F0-9]{0,4})$/.test(
- input)) {
- return 6;
- } else {
- return 0;
- }
+ return cares.isIP(input);
};
View
16 src/cares_wrap.cc
@@ -812,6 +812,19 @@ void AfterGetAddrInfo(uv_getaddrinfo_t* req, int status, struct addrinfo* res) {
delete req_wrap;
}
+static Handle<Value> isIP(const Arguments& args) {
+ HandleScope scope;
+ char address_buffer[sizeof(struct in6_addr)];
+
+ String::AsciiValue ascii(args[0]);
+ const char * ip = *ascii;
+ if (uv_inet_pton(AF_INET, ip, &address_buffer).code == UV_OK) {
+ return scope.Close(v8::Integer::New(4));
+ } else if (uv_inet_pton(AF_INET6, ip, &address_buffer).code == UV_OK) {
+ return scope.Close(v8::Integer::New(6));
+ }
+ return scope.Close(v8::Integer::New(0));
+}
static Handle<Value> GetAddrInfo(const Arguments& args) {
HandleScope scope;
@@ -886,7 +899,8 @@ static void Initialize(Handle<Object> target) {
NODE_SET_METHOD(target, "getHostByName", QueryWithFamily<GetHostByNameWrap>);
NODE_SET_METHOD(target, "getaddrinfo", GetAddrInfo);
-
+ NODE_SET_METHOD(target, "isIP", isIP);
+
target->Set(String::NewSymbol("AF_INET"), Integer::New(AF_INET));
target->Set(String::NewSymbol("AF_INET6"), Integer::New(AF_INET6));
target->Set(String::NewSymbol("AF_UNSPEC"), Integer::New(AF_UNSPEC));
View
11 test/simple/test-net-isip.js
@@ -36,6 +36,15 @@ assert.equal(net.isIP('2001:dead::'), 6);
assert.equal(net.isIP('2001:dead:beef::'), 6);
assert.equal(net.isIP('2001:dead:beef:1::'), 6);
assert.equal(net.isIP('ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff'), 6);
+assert.equal(net.isIP(':2001:252:0:1::2008:6:'), 0);
+assert.equal(net.isIP(':2001:252:0:1::2008:6'), 0);
+assert.equal(net.isIP('2001:252:0:1::2008:6:'), 0);
+assert.equal(net.isIP('2001:252::1::2008:6'), 0);
+assert.equal(net.isIP('::2001:252:1:2008:6'), 6);
+assert.equal(net.isIP('::2001:252:1:1.1.1.1'), 6);
+assert.equal(net.isIP('::2001:252:1:255.255.255.255'), 6);
+assert.equal(net.isIP('::2001:252:1:255.255.255.255.76'), 0);
+assert.equal(net.isIP('::anything'), 0);
assert.equal(net.isIP('::1'), 6);
assert.equal(net.isIP('::'), 6);
assert.equal(net.isIP('0000:0000:0000:0000:0000:0000:12345:0000'), 0);
@@ -49,4 +58,4 @@ assert.equal(net.isIPv4('2001:252:0:1::2008:6'), false);
assert.equal(net.isIPv6('127.0.0.1'), false);
assert.equal(net.isIPv6('example.com'), false);
-assert.equal(net.isIPv6('2001:252:0:1::2008:6'), true);
+assert.equal(net.isIPv6('2001:252:0:1::2008:6'), true);
Something went wrong with that request. Please try again.