Fixing parsing of DCC from an ipv6 address #126

Closed
wants to merge 3 commits into
from

Projects

None yet

2 participants

No description provided.

Owner

Where is the usage of IPv6 in DCC specified? The DCC "specification" only considers IPv4. A single implementation just dumping the IPv6 IP as a string in there doesn't mean that's the correct way.

@dominikh dominikh commented on an outdated diff Jul 17, 2013
lib/cinch/irc.rb
process_dcc_send($1 || $2, $3, $4, $5, msg, events)
end
+
+ #if msg.message =~ /^\001DCC SEND (?:"([^"]+)"|(\S+)) (\d+) (\d+)(?: (\d+))?\001$/
+ # process_dcc_send($1 || $2, $3, $4, $5, msg, events)
+ #end
end
dominikh
dominikh Jul 17, 2013 Owner

Commented code in a commit, really?

@dominikh dominikh commented on an outdated diff Jul 17, 2013
lib/cinch/irc.rb
end
# @since 2.0.0
def process_dcc_send(filename, ip, port, size, m, events)
- ip = ip.to_i
- ip = [24, 16, 8, 0].collect {|b| (ip >> b) & 255}.join('.')
+ begin
+ ip = !!!Integer(ip)
+ ip = [24, 16, 8, 0].collect {|b| (ip >> b) & 255}.join('.')
+ rescue ArgumentError, TypeError
+ @bot.loggers.debug "IP is an ipv6 address"
+ end
port = port.to_i
dominikh
dominikh Jul 17, 2013 Owner

Please rewrite this to

  1. Not use !!!Integer(...), that's honestly too much back and forth
  2. Not use exceptions for control flow. Just check if it's a number by applying a regexp or another way that avoids exceptions.

In the way it's currently written it is hard to tell apart the two code paths for IPv4 vs IPv6 addresses.

dominikh
dominikh Jul 17, 2013 Owner

Actually, have you tested this with the IPv4 case at all? !!!Integer(ip) will either raise an exception (your IPv6 case) or it will set ip = false, the IPv4 case. I don't see how line 559 could turn false into a human-readable IPv4 address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment