Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

This fixes the IPv6-Issue #90 for me. #91

Merged
merged 2 commits into from

5 participants

@ccoenen

I am not sure if it breaks anything, though. My hubot still works fine.

@ccoenen

Nope it's not stable. For some reason, my hubot just crashed with this stacktrace. :-(

TypeError: Cannot read property 'users' of undefined
  at Client.<anonymous> (/srv/hubot-repo/node_modules/hubot-irc/node_modules/irc/lib/irc.js:330:35)
  at Client.emit (events.js:67:17)
  at /srv/hubot-repo/node_modules/hubot-irc/node_modules/irc/lib/irc.js:517:22
  at Array.forEach (native)
  at Socket.<anonymous> (/srv/hubot-repo/node_modules/hubot-irc/node_modules/irc/lib/irc.js:514:15)
  at Socket.emit (events.js:67:17)
  at TCP.onread (net.js:362:31)
@martynsmith
Owner

Do you know of a public IRC server I can connect to to try this out ?

@martynsmith
Owner

Better still, since I don't easily have any client capable of ipv6 at the moment, if you could collect some raw data and paste it here, that'd be really handy :-)

If you put something like:

console.log('parseMessage(', line, ')');

At the start of the parseMessage function, and then just pasted some of the raw output from whois queries, I think I could poke around and see what needs fixing.

Thanks,

@martynsmith
Owner

With all the experimentation I've done, it seems that your patch works fine.

Are you sure the hubot stacktrace you got wasn't related to some other issue?

@ccoenen

I'm collecting a few instances. So far, it has crashed on me twice. I haven't yet looked into it (so far it works fair enough for a proof-of-concept).

i'll post the incoming lines as soon as i can get to it. Thanks for the support so far!

@martynsmith
Owner

No problems.

I might hold off merging your pull request until you've collected a bit more information.

Let me know how things go.

@ccoenen

Here's the output of one whois-attempt. I just replaced the actual domain with "domain"

parseMessage( :domain 311 hubert cc amenthes 0::ffff:192.168.1.104 * :purple )
parseMessage( :domain 319 hubert cc :#debug  )
parseMessage( :domain 312 hubert cc domain :Inspircd )
parseMessage( :domain 320 hubert cc :is using a secure connection )
parseMessage( :domain 317 hubert cc 1 1339659779 :seconds idle, signon time )
parseMessage( :domain 318 hubert cc :End of /WHOIS list. )

i'll keep the console.log in place for the next few days. If it continues to crash, i'll let you know.

@ccoenen

well... i started recording all my network traffic and wrote extensive logfiles. But over the past week the node-irc hasn't crashed once! It was possibly just some random coincidence.

I'll keep the logs/recordings in place for a few weeks (just in case).

@ccoenen

Awesome. Just as i was writing the last comment, i realize that hubot is no longer in the channel :-/

Stacktrace:

repo/node_modules/hubot-irc/node_modules/irc/lib/irc.js:520
                    throw err;
                          ^
TypeError: Cannot read property 'users' of undefined
    at Client.<anonymous> (repo/node_modules/hubot-irc/node_modules/irc/lib/irc.js:330:35)
    at Client.emit (events.js:67:17)
    at repo/node_modules/hubot-irc/node_modules/irc/lib/irc.js:517:22
    at Array.forEach (native)
    at Socket.<anonymous> (repo/node_modules/hubot-irc/node_modules/irc/lib/irc.js:514:15)
    at Socket.emit (events.js:67:17)
    at TCP.onread (net.js:362:31)

Last parseMessages:

parseMessage( PING :i.irc.meso.net )
parseMessage( PING :i.irc.meso.net )
parseMessage( :tb!tobl@0::ffff:192.168.1.69 PART #debug )

I can now safely reproduce the bug by just typing /part into my IRC-Client. I also tested the same thing before my patch and it does not crash without my modification.

In essence this means: Hold off merging a little longer. I'll look into it.

@ccoenen

Okay, i traced it back to https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L329 - here the chanData() call will return undefined, which is why the subsequent delete will fail.

@ccoenen

aaaah: the this.chans object in https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L441 looks like this in my test:

{
  ":#debug":{
    "key": ":#debug",
    "serverName": ":#debug",
    "users": {},
    "mode": ""
  }
}

note the leading colon of the key and servername fields. I guess my patch is really not ready for primetime, yet.

@ccoenen

The error appears to be when the bot is joining. The line in question:

:Hubot!nodebot@0::ffff:192.168.1.104 JOIN :#debug

parses to

{
  "prefix": "Hubot!nodebot@0::ffff:192.168.1.104",
  "nick": "Hubot",
  "user": "nodebot",
  "host": "0::ffff:192.168.1.104",
  "command": "JOIN",
  "rawCommand": "JOIN",
  "commandType": "normal",
  "args": [
    ":#debug"
  ]
}
@ccoenen

(thinking aloud ahead, you may skip over large parts of this :-D)

In https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L735 the rest of the aforementioned line will be

":#debug"

by then, the so-far-parsed message will be

{
  "prefix": "Hubot!nodebot@0::ffff:192.168.1.104",
  "nick": "Hubot",
  "user": "nodebot",
  "host": "0::ffff:192.168.1.104",
  "command": "JOIN",
  "rawCommand": "JOIN",
  "commandType": "normal",
  "args": []
}

It might be fixed by changing the replace statement in https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L724 to insert one singular whitespace instead of empty string:

    line = line.replace(/^[^ ]+ +/, ' ');

But then the channel won't get created in the first place (i.e. it never gets to line https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L305 ), because the self.nick was not set (it is an empty string in my case. It is correctly set in the connect-listener, but then overwritten in the listener for 'raw' in case 001. The line that causes it is this:

:domain.name.net 001 Hubot :Welcome to the IRC Network Hubot!nodebot@0::ffff:192.168.1.104

which parses to

{
  prefix: 'domain.name.net',
  server: 'domain.name.net',
  command: '001',
  rawCommand: '001',
  commandType: 'normal',
  args: [
   '',
   'Hubot',
   'Welcome to the IRC Network Hubot!nodebot@0::ffff:192.168.1.104'
  ]
}

the leading empty string in args comes from the message.args = middle.split(/ +/);, which in turn parses rubbish because of the empty space in front i introduced with the altered line.replace earlier.

Long story short: i'll need to come up with a better patch.

@ccoenen

i came up with a solution that uses a regex instead of the indexOf. It survives the /part that killed the previous approach, but also works with all the collected lines i discussed here and in #90.

@ccoenen

I've been running this version since my last commit. Since it didn't crash or cause any problems, i think it might be worth merging.

@martynsmith martynsmith merged commit c60b696 into martynsmith:master
@tuhoojabotti

This breaks PRIVMSGs like "hihi :)" they are parsed to ":hihi".

thanks for the heads-up. I'll look into it!

Correct this breaks PRIVMSGs and any other IRC messages with colons in them at any random place. 321e527 fixes this by pushing a space infront of IRC messages and looking for ' :' rather than ':', avoiding ipv6 hostnames being parsed, please review it.

I confirm that this introduced the bug described by tuhoojabotti. I also confirm that the fix of n0valyfe works in regards of parsing messages containing colons.

This has been fixed in #128, consider testing it. Hopefully it will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 12, 2012
  1. @ccoenen

    This fixes the IPv6-Issue #90 for me. I am not sure if it breaks anyt…

    ccoenen authored
    …hing, though. My hubot still works fine.
Commits on Jun 20, 2012
  1. @ccoenen
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 3 deletions.
  1. +3 −3 lib/irc.js
View
6 lib/irc.js
@@ -780,9 +780,9 @@ function parseMessage(line, stripColors) { // {{{
// Parse parameters
if ( line.indexOf(':') != -1 ) {
- var index = line.indexOf(':');
- middle = line.substr(0, index).replace(/ +$/, "");
- trailing = line.substr(index+1);
+ match = line.match(/(.*)(?:^:|\s+:)(.*)/);
+ middle = match[1].trimRight();
+ trailing = match[2];
}
else {
middle = line;
Something went wrong with that request. Please try again.