-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Socket::IPaddress.v4
, .v6
, .v4_mapped_v6
#13422
Add Socket::IPaddress.v4
, .v6
, .v4_mapped_v6
#13422
Conversation
Socket.v4
, .v6
, .v4_mapped_v6
Socket::IPaddress.v4
, .v6
, .v4_mapped_v6
# windows returns the former which, while correct, is not canonical | ||
# TODO: implement also `#to_s` in Crystal | ||
{Socket::IPAddress.new("::ffff:0:0", 0), Socket::IPAddress.new("::ffff:0.0.0.0", 0)}.should contain Socket::IPAddress.v4_mapped_v6(UInt8.static_array(0, 0, 0, 0), 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor detail, but this expectation is a bit confusing.
How about being more explicit about which result is expected?
# windows returns the former which, while correct, is not canonical | |
# TODO: implement also `#to_s` in Crystal | |
{Socket::IPAddress.new("::ffff:0:0", 0), Socket::IPAddress.new("::ffff:0.0.0.0", 0)}.should contain Socket::IPAddress.v4_mapped_v6(UInt8.static_array(0, 0, 0, 0), 0) | |
# windows returns the former which, while correct, is not canonical | |
# TODO: implement also `#to_s` in Crystal | |
Socket::IPAddress.v4_mapped_v6(UInt8.static_array(0, 0, 0, 0), 0).should eq Socket::IPAddress.new({{ flag?(:win32) }} ? "::ffff:0:0" : ::ffff:0.0.0.0", 0) |
I'm happy either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the previous Socket.ip?
spec, I don't have proof that Windows actually returns the non-canonical IP address on all OS versions, so it is better to err on the side of caution
sin_port: endian_swap(port), | ||
sin_addr: LibC::InAddr.new(s_addr: endian_swap(addr_value)), | ||
) | ||
new(pointerof(addr), sizeof(typeof(addr))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HertzDevil I just noticed this now during the review of #13463.
Taking a pointer to a variable on the stack is dangerous. It seems to be working here, but it's fragile.
It probably only works because .new
and #initialize
don't allocate enough variables to override the (relevant) parts on the stack.
I suppose a safer alternative could perhaps be a yielding variant of #initialize
or some other way to directly assign to the memory of the Address
struct.
Same applies for v6
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a tail call here (I believe you have to opt in explicitly in the LLVM IR), so addr
will be valid before new
returns. The #initialize
bodies exclusively dereference addr
and do not attempt to extend its lifetime.
In any case I plan to make SockaddrIn
or SockaddrIn6
part of the IPAddress
itself so that #to_unsafe
never allocates. That would allow addr
to be passed by value here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, if LLVM keeps this safe, then it's fine.
Resolves #13382.