Fixing ipv6 parsing and messages with colons in them. #128

Closed
wants to merge 4 commits into
from

Projects

None yet

5 participants

@rickihastings

As mentioned in af75124#commitcomment-1956569 this commits breaks messages containing colons anywhere. As you can see in the image below this is parsed using irc.js with that version, it also occasionally crashes on ipv6.

1cQK5

I've managed to fix it by checking for a space then a colon, as per the IRC RFC http://www.ietf.org/rfc/rfc1459.txt a colon has to be preceded by a space and as far as I know always is because the command is delimited by spaces. So theres no reason for any fancy regex, we can just force a space in at the start if needed and then look for ' :' because an ipv6 hostname can't contain any spaces.

Please review!

rickihastings added some commits Jan 5, 2013
@rickihastings rickihastings Fixes ipv6 and doesn't screw up privmsgs with colons 321e527
@rickihastings rickihastings Checking prior to inserting the space
Some messages in the IRC protocol can come in without being prepended with a :, a space then a non colon may break someones existing code relying on it being parsed properly.
9168aab
@ryancole
ryancole commented Jan 6, 2013

I can confirm this bug exists. If, on IRC, somebody sends the following message "test :1 :2 :3", the code shows that the message was ":test". So, it fails to cut off the leading colon and it's not greedy enough in reading to the end of the message line, and leaves off the ":1 :2 :3".

@rickihastings

Yes I was producing similar results. I can confirm the changes I've made
make sure this does not happen and parsing ipv6 causes no problems.
Probably needs a bit more testing with commands containing hostnames.

@ryancole
ryancole commented Jan 7, 2013

I'll try out your patches, but I'd like to see this make it into master.

@rickihastings

After running this patch for a couple of days I've noticed a bug with my app, traced it back to this. 'MODE #channel +o Nickname' was being parsed into '+o Nickname'. I traced it down to this line https://github.com/n0valyfe/node-irc/blob/321e527764906e146f70681654265c08967d68a9/lib/irc.js#L881

I'm not sure what this line does, looks like it could be trimming whitespace, if it is it would probably make more sense to do middle = line.trim();

@qsheets
Contributor
qsheets commented Jan 10, 2013

Let's make this simple (starting at lib/irc.js:875):

if ( line.indexOf(':') != -1 ) {
    match = line.match(/(.*)(?:^:|\s+:)(.*)/);
    middle = match[1].trimRight();
    trailing = match[2];
}

Changes to:

if ( line.indexOf(':') != -1 ) {
    match = line.match(/((?:(?! :).)*)(?:^:|\s+:)(.*)/);
    middle = match[1].trimRight();
    trailing = match[2];
}

Works like a charm for me. Any questions?

@martynsmith
Owner

Or we could just make the first match non-greedy:

   match = line.match(/(.*?)(?:^:|\s+:)(.*)/);
@martynsmith
Owner

More importantly, can someone paste some sample lines that this is supposed to be parsing so we can look at them and compare how each implementation fares?

@rickihastings
if ( line.indexOf(':') != -1 ) {
    match = line.match(/((?:(?! :).)*)(?:^:|\s+:)(.*)/);
    middle = match[1].trimRight();
    trailing = match[2];
}

Seems to work fine actually, the code in master didn't at all for me and crashed during commands containing ipv6 hosts and put colons from messages where ever it liked. Not sure if it was something to do with the node version or client implementation, doesn't seem like it though. I may need to test more but the code above seems to be working with both ipv6 commands and PRIVMSG like commands containing colons.

@damianb
Contributor
damianb commented Jan 10, 2013

FWIW, I do believe oftc supports full IPv6 and has clients connected via ipv6 on a regular basis. Might be a good network to test on, perhaps.

@rickihastings

Yes, I've been sitting a client in a number of large freenode channels for a while now and had no crashes related to the parsing of ipv6. Also colons are being parsed fine, not sure if anyone else is having problems with the previous commits attempting to fix this in master. Full parsing seems to be working fine for me now.

@damianb
Contributor
damianb commented Jan 11, 2013

As I stated, current HEAD of master (2faf5a5#commitcomment-2415429) doesn't completely fix the issue.

@rickihastings

I can confirm the changes in this pull request fix it. Or, alternatively these changes below (I think, may need more testing)

if ( line.indexOf(':') != -1 ) {
    match = line.match(/((?:(?! :).)*)(?:^:|\s+:)(.*)/);
    middle = match[1].trimRight();
    trailing = match[2];
}

Although this isn't currently in master, the regex in master seems to be broken.

rickihastings added some commits Jan 13, 2013
@rickihastings rickihastings Fixes colon and ipv6 bugs.
Been running this for a couple of days with no problems, happy with it. Hopefully ready to be merged.
4b66f46
@rickihastings rickihastings Removing redundant code 217f0ef
@rickihastings

I've just had my first crash relating to this code in days. I'm currently using the version in the my master branch, which is the one @qsheets posted 4 days ago.

            middle = match[1].trimRight();
                          ^
            TypeError: Cannot read property '1' of null

The crash occured when parsing an ipv6 hostname from /WHO. Any ideas? I was confident that code would fix it. I'm rolling back to the initial commits I made until this is fixed and pushed to master.

@qsheets
Contributor
qsheets commented Jan 14, 2013

@n0valyfe use the line Martyn wrote.

@martynsmith
Owner

In my brief testing, the line I wrote is broken ...

I'll have another look when I get a chance

On Tue, Jan 15, 2013 at 11:18 AM, qsheets notifications@github.com wrote:

@n0valyfe https://github.com/n0valyfe use the line Martyn wrote.


Reply to this email directly or view it on GitHubhttps://github.com/martynsmith/node-irc/pull/128#issuecomment-12243026.

@rickihastings

Yeah neither lines work. They seem to crash on parsing of ipv6 addresses, It's odd I had it running in a few large freenode channels it only crashed in one though, all of them had ipv6 users. The offending channel was #Node.js, so unless the regex picked up the . in the channel name? I'm not sure I need to do more testing.

@ryancole

For me, this has nothing to do with IPv6, and it just has everything to do with the regex used to parse chat messages. The regex fails when a message to a channel has colons in it. This is easy to test: use the current master branch, and see what happens when somebody else sends the following message "test :1 :2 :3". The message will be parsed by this module and returned as ":test". I'm not having any crashes or anything, just incorrectly parsed messages.

@qsheets qsheets added a commit to qsheets/node-irc that referenced this pull request Jan 19, 2013
@qsheets qsheets Fix for receiving messages with colons
This fixes issues #122, #128 and #133.

Example lines include:
    :some.irc.net 324 webuser #channel +Cnj 5:10
    :nick!user@host QUIT :Ping timeout: 252 seconds
    :nick!user@host PRIVMSG #channel :so : colons: :are :: not a problem ::::
af6f07f
@rickihastings

I'm not sure where the crash is coming from then. It just appears to occasionally happen in channels with ipv6 users in on WHO. I'll apply the new patch and see if I can find any problems with it.

@martynsmith
Owner

Fairly sure #133 fixes this issue.

@uiureo uiureo added a commit to uiureo/node-irc that referenced this pull request Feb 2, 2013
@qsheets qsheets Fix for receiving messages with colons
This fixes issues #122, #128 and #133.

Example lines include:
    :some.irc.net 324 webuser #channel +Cnj 5:10
    :nick!user@host QUIT :Ping timeout: 252 seconds
    :nick!user@host PRIVMSG #channel :so : colons: :are :: not a problem ::::
b64f870
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment