Socket.gethostbyname #2125

Closed
wants to merge 6 commits into
from

Projects

None yet

2 participants

@sdaubert
Contributor

Update Socket.gethostbyname.

First, spec is updated to add tests on address list to verify that these addresses are in packed format.
In a second commit Socket.gethostbyname is updated for all ruby versions to pass spec.

sdaubert added some commits Jan 14, 2013
@sdaubert sdaubert Update spec library/socket/socket/gethostbyname.
Add two new specs to verify that addresses in address list returned
by Socket.gethosbyname are in 'pack' format (one spec for IPv4
addresses an the other for IPv6 ones).
45a1371
@sdaubert sdaubert Update Socket.gethostbyname to respect two new specs.
Socket.gethostbyname is updated for all ruby versions.

Socket.sockaddr_in is used to transform addresses got from
Socket.getaddrinfo to packed strings. For an IPv4 address,
address is from 5th byte and is 4 bytes long. For an IPv6
address, address is from 8th byte and is 16 bytes long.
9bda567
@dbussink dbussink and 1 other commented on an outdated diff Jan 14, 2013
lib/18/socket.rb
@@ -610,7 +610,17 @@ def self.gethostbyname(hostname)
alternatives = []
addrinfos.each do |a|
alternatives << a[2] unless a[2] == hostname
- addresses << a[3] if a[4] == family
+ # transform addresses to packed strings
+ if a[4] == family
+ sockaddr = Socket.sockaddr_in(1, a[3])
+ if sockaddr.length == 16
@dbussink
dbussink Jan 14, 2013 Member

We shouldn't just check the length and thus assume a certain layout. It's better to switch on the family, whether it's AF_INET or AF_INET6

@sdaubert
sdaubert Jan 15, 2013 Contributor

Good point.

@dbussink dbussink and 1 other commented on an outdated diff Jan 14, 2013
lib/18/socket.rb
@@ -610,7 +610,17 @@ def self.gethostbyname(hostname)
alternatives = []
addrinfos.each do |a|
alternatives << a[2] unless a[2] == hostname
- addresses << a[3] if a[4] == family
+ # transform addresses to packed strings
+ if a[4] == family
+ sockaddr = Socket.sockaddr_in(1, a[3])
+ if sockaddr.length == 16
+ # IPv4 address
+ addresses << sockaddr[4, 4]
@dbussink
dbussink Jan 14, 2013 Member

We should also find another way of know which bytes are what. We can't be sure about the exact byte layouts of these so we shouldn't hard code these offsets here.

@sdaubert
sdaubert Jan 15, 2013 Contributor

But these offsets are not magic : they come from C struct definitions:

/* man 7 ip */
           struct sockaddr_in {
               sa_family_t    sin_family;   /* AF_INET */
               in_port_t      sin_port;      /* port number */
               struct in_addr sin_addr;   /* internet address */
           };

           struct in_addr {
               uint32_t       s_addr;
           };

and

/* man 7 ipv6 */
           struct sockaddr_in6 {
               sa_family_t     sin6_family;   /* AF_INET6 */
               in_port_t       sin6_port      /* port number */;
               uint32_t        sin6_flowinfo; /* flow information */
               struct in6_addr sin6_addr;     /* IPv6 adress */
               uint32_t        sin6_scope_id; /* Scope ID */
           };

           struct in6_addr {
               unsigned char   s6_addr[16];
           };
@dbussink
dbussink Jan 15, 2013 Member

That's exactly my point :). We should get these offsets from struct definitions for these types and not hard code these offsets. They might be different due to platform alignment, type sizes etc. that's why we should depend on what information the platform supplies for these types.

@sdaubert
sdaubert Jan 15, 2013 Contributor

OK. So we can use Socket::SockAddr_In. SockAddr_In#offset_of gives offset of sockaddr_in* fields. But i don't known how to get their size.

@sdaubert
Contributor

I have studied Rubinius::FFI, and I think I understand the problem :

  • struct sockaddr_in6 must be defined in rakelib/platform.rake.
  • FFI.config must be used to obtain offsets and sizes for sockaddr_in* fields.

I will try to do that this week.

@dbussink
Member

Yeah, that sounds like the direction I was also thinking about :). If you have any questions, don't hesitate to ask :). You can also try hopping into #rubinius on IRC to see if people are around there.

@sdaubert sdaubert Pass spec gethostbyname cleanly.
* rakelib/platform.rake:
  Add new structure `sockaddr_in6`
* lib/**/socket.rb :
  * Modify test to determine IP version. Use family
    instead of sockaddr string size.
  * Use platform configuration to determine offset and
    size of IP address in sockaddr string. `sockaddr_in`
    and `sockaddr_in6` platform structures are used for
    this purpose.
77a888c
@sdaubert
Contributor

Yeah, it works !
Last commit uses platform layout to get packed address from sockaddr string.

@dbussink dbussink and 1 other commented on an outdated diff Jan 18, 2013
lib/18/socket.rb
@@ -610,7 +610,23 @@ def self.gethostbyname(hostname)
alternatives = []
addrinfos.each do |a|
alternatives << a[2] unless a[2] == hostname
- addresses << a[3] if a[4] == family
+ # transform addresses to packed strings
+ if a[4] == family
+ sockaddr = Socket.sockaddr_in(1, a[3])
+ if family == AF_INET
+ # IPv4 address
+ offset = FFI.config("sockaddr_in.sin_addr.offset")
+ size = FFI.config("sockaddr_in.sin_addr.size")
+ addresses << sockaddr[offset, size]
+ elsif family == AF_INET6
+ # Ipv6 address
+ offset = FFI.config("sockaddr_in6.sin6_addr.offset")
+ size = FFI.config("sockaddr_in6.sin6_addr.size")
+ addresses << sockaddr[8,16]
@dbussink
dbussink Jan 18, 2013 Member

Looks like you're not using the offsets here, but have the hardcoded values left over :)

@dbussink
dbussink Jan 18, 2013 Member

BTW, since these should be interpreter as binary strings, it's probably better to use byteslice instead of String#[]

@sdaubert
sdaubert Jan 18, 2013 Contributor

Oops ! I missed one.
And i'll add #byteslice.

@sdaubert sdaubert Socket.gethostbyname
* Remove hard-coded offset and size for IPv6.
* Replace String#[] by String#byteslice to get address
  from sockaddr string.
e10cc24
@dbussink
Member

So, one final question, could you perhaps squash the implementation commits into a single commit? So the pull request is 2 commits, one for the specs, one for the implementation?

@sdaubert
Contributor

I don't know how doing that... I am a git newbie.

sdaubert added some commits Jan 14, 2013
@sdaubert sdaubert Update Socket.gethostbyname to respect two new specs.
Socket.gethostbyname is updated for all ruby versions.

* rakelib/platform.rake:
  Add new structure `sockaddr_in6`

* lib/**/socket.rb :
  Socket.sockaddr_in is used to transform addresses got
  from Socket.getaddrinfo to packed strings. Platform
  configuration is then used to determine offset and size
  of IP address in sockaddr string. `sockaddr_in` and
  `sockaddr_in6` platform structures are used for this
  purpose.
e956c23
@sdaubert sdaubert Merge branch 'socket.gethostbyname' of github.com:sdaubert/rubinius i…
…nto socket.gethostbyname
f5e460f
@sdaubert
Contributor

I tried something with git rebase, but i'm not sure that's good.
May i do that in another branch ?

@dbussink
Member

git rebase should work yeah, if you do an interactive rebase you can squash commits together. If you have it right locally, you can force push it to overwrite the changes in the pull request.

Otherwise I can merge it locally on my machine and squash it there.

@sdaubert
Contributor

It seems I made a mess here. Please merge it locally on your machine.

@dbussink
Member

Merged this manually

@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