Socket.gethostbyname respond with "<any>" and "<broadcast>" hosts #2111

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
@sdaubert
Contributor

sdaubert commented Dec 31, 2012

Make specs "Socket#gethostbyname returns broadcast address info for ''" and "Socket#gethostbyname returns broadcast address info for ''" pass
See commit messages for details.

sdaubert added some commits Dec 30, 2012

Spec "Socket#gethostbyname returns broadcast address info for '<any>'…
…" is OK.

Socket.getaddrinfo is modified to act the same way when +host+ is "" or is
"<any>".
Socket.gethostbyname is modified before returning : addresses array is modified
to return packed-string addresses instead of IPv4 dotted addresses.
Make spec "Socket#gethostbyname returns broadcast address info for '<…
…broadcast>'" pass.

Socket::Foreign.getaddrinfo is modified to use "<broadcast>" as "255.255.255.255" (first
+if+ statement).
@dbussink

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Dec 31, 2012

Member

Few comments, first of all, why was this only added for 1.9 mode? This seems like a feature that should be available also in 1.8, since there are also tags in 1.8 mode for this.

Member

dbussink commented Dec 31, 2012

Few comments, first of all, why was this only added for 1.9 mode? This seems like a feature that should be available also in 1.8, since there are also tags in 1.8 mode for this.

@@ -688,6 +690,16 @@ def self.gethostbyname(hostname)
addresses << a[3] if a[4] == family
end
+ addresses.map! do |addr|

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Dec 31, 2012

Member

Could you explain what this code does? Personally I don't find it very readable, maybe there is a way we can make this cleaner / more obvious as to what it does?

@dbussink

dbussink Dec 31, 2012

Member

Could you explain what this code does? Personally I don't find it very readable, maybe there is a way we can make this cleaner / more obvious as to what it does?

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Dec 31, 2012

Contributor

This code transforms a dotted IPv4 address into a packed address, as returned by MRI.
First, if addr seems to be an IPv4 address, each address byte is packed into string s. Else addr is returned unchanged.

Now, for packing, i think it should use pack:

$~[1..4].map(:to_i).pack("C*")

instead of

$~[1..4].each { |v| s << v.to_i.chr }
@sdaubert

sdaubert Dec 31, 2012

Contributor

This code transforms a dotted IPv4 address into a packed address, as returned by MRI.
First, if addr seems to be an IPv4 address, each address byte is packed into string s. Else addr is returned unchanged.

Now, for packing, i think it should use pack:

$~[1..4].map(:to_i).pack("C*")

instead of

$~[1..4].each { |v| s << v.to_i.chr }
@sdaubert

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Dec 31, 2012

Contributor

I plan to add it for 1.8 mode after it will be accepted for 1.9. I am not confident about some modifications, like line 693.

Contributor

sdaubert commented Dec 31, 2012

I plan to add it for 1.8 mode after it will be accepted for 1.9. I am not confident about some modifications, like line 693.

@dbussink

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Dec 31, 2012

Member

Ah ok, we would prefer to have them in a single pull request then, so the changes can be made together. I suspect the changes will be exactly the same anyway, but we can work on reviewing this change first then if you are more comfortable with that.

I don't really have much time today anymore though, you know, something with 2013 ;). Have a good new year, and I'll take a further look later this week :).

Member

dbussink commented Dec 31, 2012

Ah ok, we would prefer to have them in a single pull request then, so the changes can be made together. I suspect the changes will be exactly the same anyway, but we can work on reviewing this change first then if you are more comfortable with that.

I don't really have much time today anymore though, you know, something with 2013 ;). Have a good new year, and I'll take a further look later this week :).

@sdaubert

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Dec 31, 2012

Contributor

I agree you about changes on 1.8 version, but I would prefer reviewing this change first.
Thanks.

Contributor

sdaubert commented Dec 31, 2012

I agree you about changes on 1.8 version, but I would prefer reviewing this change first.
Thanks.

@sdaubert

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Jan 1, 2013

Contributor

As MRI also returns a packed string for IPv6 address, i propose to modify code as:

    addresses.map! do |addr|
      if addr =~ /\./
        # IPv4 address
        addr.split('.').map { |v| v.to_i }.pack('C*')
      elsif addr =~ /:/
        # IPv6 address
        left, right = addr.split('::')
        l = left.split(':')
        r = right.split(':')
        rest = 8 - l.size - r.size
        ary = l + ['0'] * rest + r
        ary.map { |v| v.to_i }.pack('n8')
      else
        # other (?): return addr as is
        addr
      end
Contributor

sdaubert commented Jan 1, 2013

As MRI also returns a packed string for IPv6 address, i propose to modify code as:

    addresses.map! do |addr|
      if addr =~ /\./
        # IPv4 address
        addr.split('.').map { |v| v.to_i }.pack('C*')
      elsif addr =~ /:/
        # IPv6 address
        left, right = addr.split('::')
        l = left.split(':')
        r = right.split(':')
        rest = 8 - l.size - r.size
        ary = l + ['0'] * rest + r
        ary.map { |v| v.to_i }.pack('n8')
      else
        # other (?): return addr as is
        addr
      end
@hron84

This comment has been minimized.

Show comment Hide comment
@hron84

hron84 Jan 2, 2013

@sdaubert I worry about your solution. IPv6 address can contain double colon, but it is not a requirement. If IPv6 address is written in fully-expanded form, there is no double colon.

Ohh, and simple shortening:

addr.split('.').map(&:to_i).pack('C*')

hron84 commented Jan 2, 2013

@sdaubert I worry about your solution. IPv6 address can contain double colon, but it is not a requirement. If IPv6 address is written in fully-expanded form, there is no double colon.

Ohh, and simple shortening:

addr.split('.').map(&:to_i).pack('C*')
@sdaubert

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Jan 2, 2013

Contributor

@hron84 you are right. To make this code work, we have to add

right ||= ''

after

left, right = addr.split('::')

I also discovered a bug by testing fully expanded form. We must use

ary.map { |v| v.to_i(16) }.pack('n8')

instead of

ary.map { |v| v.to_i }.pack('n8')
Contributor

sdaubert commented Jan 2, 2013

@hron84 you are right. To make this code work, we have to add

right ||= ''

after

left, right = addr.split('::')

I also discovered a bug by testing fully expanded form. We must use

ary.map { |v| v.to_i(16) }.pack('n8')

instead of

ary.map { |v| v.to_i }.pack('n8')
Modify end of Socket.gethostbyname to also convert IPv6 addresses into
packed strings. This code is taken from IPAddr#hton.
@sdaubert

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Jan 3, 2013

Contributor

I added commit 90534f2 for supporting IPv6 addresses (and also dotted format for IPv4-mapped and IPv4-compatible IPv6 addresses).

Contributor

sdaubert commented Jan 3, 2013

I added commit 90534f2 for supporting IPv6 addresses (and also dotted format for IPv4-mapped and IPv4-compatible IPv6 addresses).

@dbussink

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Jan 4, 2013

Member

Ok, there are two concerns here. The one with handling and and the fact that Socket.gethostbyname doesn't return the addresses encoded in the same way as MRI. We should not be mixing these two issues here probably.

For the second issue, this is not the way we should approach fixing it. Doing this with manual regexp parsing is error prone and we should be using SockAddr_In and probably add the SockAddr_In6 struct for that. Those are the canonical structures that handle parsing like this, we shouldn't be copying that logic in other places.

This is not really a trivial change and probably requires much more work than this. What I suggest for this pull request, is changing the spec so it doesn't depend on matching the whole result of Socket.gethostbyname, but just on the first entry of the array that gethostbyname returns and specify that has to be "0.0.0.0" for and "255.255.255.255" for .

The formatting of the address as the last entry should be specced separate, so we can also tackle that as a separate issue.

Member

dbussink commented Jan 4, 2013

Ok, there are two concerns here. The one with handling and and the fact that Socket.gethostbyname doesn't return the addresses encoded in the same way as MRI. We should not be mixing these two issues here probably.

For the second issue, this is not the way we should approach fixing it. Doing this with manual regexp parsing is error prone and we should be using SockAddr_In and probably add the SockAddr_In6 struct for that. Those are the canonical structures that handle parsing like this, we shouldn't be copying that logic in other places.

This is not really a trivial change and probably requires much more work than this. What I suggest for this pull request, is changing the spec so it doesn't depend on matching the whole result of Socket.gethostbyname, but just on the first entry of the array that gethostbyname returns and specify that has to be "0.0.0.0" for and "255.255.255.255" for .

The formatting of the address as the last entry should be specced separate, so we can also tackle that as a separate issue.

@sdaubert

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Jan 4, 2013

Contributor

So, I make a new pull request for handling 'any' and 'broadcast' and for modifying specs to test only the first item of returned array. This pull request will handle all ruby versions.
I will try to fill another pull request later about handling last item in returned array, with specs.
Is that right ?

Contributor

sdaubert commented Jan 4, 2013

So, I make a new pull request for handling 'any' and 'broadcast' and for modifying specs to test only the first item of returned array. This pull request will handle all ruby versions.
I will try to fill another pull request later about handling last item in returned array, with specs.
Is that right ?

@dbussink

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Jan 4, 2013

Member

Right, we should handle this first case first. The second part will be a lot more complicated I guess though, I've tried to see what needed to be done, and it's quite a significant effort and probably touches a bunch of more places, like FFI struct generation etc.

Member

dbussink commented Jan 4, 2013

Right, we should handle this first case first. The second part will be a lot more complicated I guess though, I've tried to see what needed to be done, and it's quite a significant effort and probably touches a bunch of more places, like FFI struct generation etc.

@sdaubert

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Jan 4, 2013

Contributor

For the first case, see new pull request #2115.

Contributor

sdaubert commented Jan 4, 2013

For the first case, see new pull request #2115.

@sdaubert

This comment has been minimized.

Show comment Hide comment
@sdaubert

sdaubert Jan 14, 2013

Contributor

For the second issue, I propose a new solution in #2125. If #2125 is accepted, then we could close this pull request, i think.

Contributor

sdaubert commented Jan 14, 2013

For the second issue, I propose a new solution in #2125. If #2125 is accepted, then we could close this pull request, i think.

@dbussink

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Jan 21, 2013

Member

I'm closing this since it has been handled in two separate pull requests.

Member

dbussink commented Jan 21, 2013

I'm closing this since it has been handled in two separate pull requests.

@dbussink dbussink closed this Jan 21, 2013

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