Skip to content
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

[cancelled, not using Crystal anymore] Add abstract unix socket handling #4056

Closed
Closed
186 changes: 178 additions & 8 deletions spec/std/socket_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -169,15 +169,67 @@ describe Socket::IPAddress do
end

describe Socket::UNIXAddress do
{% if flag?(:linux) %}
it "can be abstract on Linux" do
path = "/abstract.sock"
addr = Socket::UNIXAddress.new('@' + path)
addr.abstract?.should be_true
addr.path.should eq(path)
end
{% else %}
it "raises for abstract UNIX address on non-Linux" do
expect_raises(ArgumentError, /not supported/) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need a regexp here, using "not supported" as argument will match if the string is included in the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I though using a String would make it a exact match check. Stdlib is well done, it's an inclusion check, thanks 😄

Copy link
Contributor Author

@bew bew Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's at least one other place in this file where a Regex is used instead of a String for a similar case, but I think it's not in this PR's scope to change that, I'm just mentioning it so that the knowledge is shared (no idea if it's helpful at all ^^)

expect_raises(Socket::Error, /Invalid IP address/) do

Socket::UNIXAddress.new("@/abstract.sock")
end
end

it "is not abstract on non-Linux" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this example should not run on linux target?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no indeed

path = "/tmp/unix.sock"
addr = Socket::UNIXAddress.new(path)
addr.abstract?.should be_false
addr.path.should eq(path)
end
{% end %}

it "can create a non-abstract UNIX address starting with @" do
path = "./@non-abstract.sock"
addr = Socket::UNIXAddress.new(path)
addr.abstract?.should be_false
addr.path.should eq(path)
end

it "transforms into a C struct and back" do
addr1 = Socket::UNIXAddress.new("/tmp/service.sock")
addr2 = Socket::UNIXAddress.from(addr1.to_unsafe, addr1.size)
path = "/tmp/service.sock"

addr1 = Socket::UNIXAddress.new(path)
addr1.abstract?.should be_false
addr1.path.should eq(path)

addr2 = Socket::UNIXAddress.from(addr1.to_unsafe, addr1.size)
addr2.family.should eq(addr1.family)
addr2.path.should eq(addr1.path)
addr2.to_s.should eq("/tmp/service.sock")
addr2.abstract?.should eq(addr1.abstract?)
addr2.to_s.should eq(path)
end

{% if flag?(:linux) %}
it "transforms an abstract address into a C struct and back" do
path = "/abstract-service.sock"

addr1 = Socket::UNIXAddress.new('@' + path)
addr1.path.should eq(path)
addr1.abstract?.should be_true

sockaddr_un = addr1.to_unsafe.as(LibC::SockaddrUn*).value
sockaddr_un.sun_path[0].should eq(0_u8)
String.new(sockaddr_un.sun_path.to_unsafe + 1).should eq(path)

addr2 = Socket::UNIXAddress.new(pointerof(sockaddr_un), nil)
addr2.path.should eq(addr1.path)
addr2.abstract?.should eq(addr1.abstract?)
end
{% end %}

it "raises when path is too long" do
path = "/tmp/crystal-test-too-long-unix-socket-#{("a" * 2048)}.sock"
expect_raises(ArgumentError, "Path size exceeds the maximum size") { Socket::UNIXAddress.new(path) }
Expand All @@ -186,6 +238,12 @@ describe Socket::UNIXAddress do
it "to_s" do
Socket::UNIXAddress.new("some_path").to_s.should eq("some_path")
end

{% if flag?(:linux) %}
it "to_s for abstract UNIX address" do
Socket::UNIXAddress.new("@some_path").to_s.should eq("@some_path")
end
{% end %}
end

describe UNIXServer do
Expand All @@ -195,6 +253,26 @@ describe UNIXServer do
File.exists?(path).should be_false
end

{% if flag?(:linux) %}
it "is not abstract when path is not an abstract address" do
path = "/tmp/crystal-test-unix-sock"

UNIXServer.open(path) do |server|
server.abstract?.should be_false
end
end
{% end %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two macro branches can be combined.


{% if flag?(:linux) %}
it "is abstract when path is an abstract address" do
path = "/tmp/crystal-test-unix-abstract-sock"

UNIXServer.open('@' + path) do |server|
server.abstract?.should be_true
end
end
{% end %}

it "creates the socket file" do
path = "/tmp/crystal-test-unix-sock"

Expand All @@ -205,6 +283,30 @@ describe UNIXServer do
File.exists?(path).should be_false
end

it "creates the socket file with path starting with @" do
path = "./@crystal-test-unix-sock-starting-with-at-symbol"

File.exists?(path).should be_false

UNIXServer.open(path) do
File.exists?(path).should be_true
end

File.exists?(path).should be_false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may get rid of the outer File.exists? expectations.

end

{% if flag?(:linux) %}
it "does not create any file for abstract server" do
path = "/tmp/crystal-test-unix-abstract-sock"

File.exists?(path).should be_false

UNIXServer.open('@' + path) do
File.exists?(path).should be_false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also doublecheck that '@' + path doesn't exist either? Might be paranoid...

end
end
{% end %}

it "deletes socket file on close" do
path = "/tmp/crystal-test-unix-sock"

Expand All @@ -217,6 +319,23 @@ describe UNIXServer do
end
end

{% if flag?(:linux) %}
it "does not delete any file on close for abstract server" do
path = "/tmp/crystal-test-close-unix-abstract-sock"

File.touch(path)
File.exists?(path).should be_true

begin
server = UNIXServer.new('@' + path)
server.close
File.exists?(path).should be_true
ensure
File.delete(path) if File.exists?(path)
end
end
{% end %}

it "raises when socket file already exists" do
path = "/tmp/crystal-test-unix-sock"
server = UNIXServer.new(path)
Expand All @@ -231,7 +350,7 @@ describe UNIXServer do
it "won't delete existing file on bind failure" do
path = "/tmp/crystal-test-unix.sock"

File.write(path, "")
File.touch(path)
File.exists?(path).should be_true

begin
Expand All @@ -247,15 +366,34 @@ describe UNIXServer do

describe "accept" do
it "returns the client UNIXSocket" do
UNIXServer.open("/tmp/crystal-test-unix-sock") do |server|
UNIXSocket.open("/tmp/crystal-test-unix-sock") do |_|
path = "/tmp/crystal-test-unix-sock"

UNIXServer.open(path) do |server|
UNIXSocket.open(path) do |_|
client = server.accept
client.should be_a(UNIXSocket)
client.abstract?.should be_false
client.close
end
end
end

{% if flag?(:linux) %}
it "returns an abstract client UNIXSocket for abstract server" do
path = "/tmp/crystal-test-abstract-unix-sock"
abstract_path = '@' + path

UNIXServer.open(abstract_path) do |server|
UNIXSocket.open(abstract_path) do |_|
client = server.accept
client.should be_a(UNIXSocket)
client.abstract?.should be_true
client.close
end
end
end
{% end %}

it "raises when server is closed" do
server = UNIXServer.new("/tmp/crystal-test-unix-sock")
exception = nil
Expand All @@ -280,8 +418,10 @@ describe UNIXServer do

describe "accept?" do
it "returns the client UNIXSocket" do
UNIXServer.open("/tmp/crystal-test-unix-sock") do |server|
UNIXSocket.open("/tmp/crystal-test-unix-sock") do |_|
path = "/tmp/crystal-test-unix-sock"

UNIXServer.open(path) do |server|
UNIXSocket.open(path) do |_|
client = server.accept?.not_nil!
client.should be_a(UNIXSocket)
client.close
Expand Down Expand Up @@ -339,6 +479,36 @@ describe UNIXSocket do
end
end

{% if flag?(:linux) %}
it "sends and receives messages over an abstract STREAM socket" do
path = "/abstract-service.sock"
abstract_path = '@' + path

UNIXServer.open(abstract_path) do |server|
server.local_address.abstract?.should be_true
server.local_address.path.should eq(path)

UNIXSocket.open(abstract_path) do |client|
client.local_address.abstract?.should be_true
client.local_address.path.should eq(path)

server.accept do |sock|
sock.local_address.path.should eq("")
sock.local_address.abstract?.should be_true

sock.remote_address.path.should eq("")
sock.remote_address.abstract?.should be_true

client << "ping"
sock.gets(4).should eq("ping")
sock << "pong"
client.gets(4).should eq("pong")
end
end
end
end
{% end %}

it "sync flag after accept" do
path = "/tmp/crystal-test-unix-sock"

Expand Down
57 changes: 53 additions & 4 deletions src/socket/address.cr
Original file line number Diff line number Diff line change
Expand Up @@ -179,20 +179,46 @@ class Socket
# Holds the local path of an UNIX address, usually coming from an opened
# connection (e.g. `Socket#local_address`, `Socket#receive`).
#
# You may also declare an abstract UNIX address, that is a virtual file
# that will never be created on the filesystem. An abstract UNIX address
# path is prefixed by a `@` character.
#
# NOTE:
# - Abstract UNIX addresses are supported only on some Linux systems.
# - If you want to use a non-abstract UNIX address starting with a `@`,
# prefix it with `./`, like `./@foo`.
#
# Example:
# ```
# Socket::UNIXAddress.new("/tmp/my.sock")
#
# # Abstract UNIX socket on Linux only
# Socket::UNIXAddress.new("@/my.sock")
# ```
struct UNIXAddress < Address
getter path : String
getter? abstract : Bool

# :nodoc:
MAX_PATH_SIZE = LibC::SockaddrUn.new.sun_path.size - 1

def initialize(@path : String)
if @path.bytesize + 1 > MAX_PATH_SIZE
def initialize(path : String)
if path.bytesize + 1 > MAX_PATH_SIZE
raise ArgumentError.new("Path size exceeds the maximum size of #{MAX_PATH_SIZE} bytes")
end

if path[0]? == '@'
Copy link
Member

@straight-shoota straight-shoota Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path.starts_with?('@') ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I didn't want to do that because unconsciously for me starts_with? is to check that it starts with a String, not a Char....

Nevermind I'll change it.

{% if flag?(:linux) %}
@abstract = true
@path = path[1..-1]
{% else %}
raise ArgumentError.new("Unsupported: cannot use abstract UNIX socket on non-Linux")
{% end %}
else
@abstract = false
@path = path
end

@family = Family::UNIX
@size = sizeof(LibC::SockaddrUn)
end
Expand All @@ -204,7 +230,20 @@ class Socket

protected def initialize(sockaddr : LibC::SockaddrUn*, size)
@family = Family::UNIX
@path = String.new(sockaddr.value.sun_path.to_unsafe)

path = sockaddr.value.sun_path
if path[0]? == 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path.starts_with?(Char::ZERO) ?

Copy link
Contributor Author

@bew bew Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path is not a String here, it's a StaticArray(UInt8).

Based on that, you made me think that it would be even more correct to check for equality with 0_u8 (even though there's an automatic conversion happening here somewhere)

{% if flag?(:linux) %}
@abstract = true
@path = String.new(path.to_unsafe + 1)
{% else %}
raise ArgumentError.new("Unsupported: cannot use abstract UNIX socket on non-Linux")
{% end %}
else
@abstract = false
@path = String.new(path.to_unsafe)
end

@size = size || sizeof(LibC::SockaddrUn)
end

Expand All @@ -213,13 +252,23 @@ class Socket
end

def to_s(io)
if abstract?
io << '@'
end
io << path
end

def to_unsafe : LibC::Sockaddr*
sockaddr = Pointer(LibC::SockaddrUn).malloc
sockaddr.value.sun_family = family
sockaddr.value.sun_path.to_unsafe.copy_from(@path.to_unsafe, @path.bytesize + 1)

destination = sockaddr.value.sun_path.to_unsafe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sun_path is a LibC::Char[?]: Use a Slice with #to_slice instead of raw #to_unsafe Pointer.

if @abstract
destination[0] = 0_u8
destination += 1
end
destination.copy_from(@path.to_unsafe, @path.bytesize + 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is the reason you should prefer Slice: Your write to destination is bound to the size of @path, and not checked to the size of destination. You get this check for free with Slice.

Copy link
Contributor Author

@bew bew Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Papierkorb !
What about @path should I keep the #to_unsafe? (I think it's not harmful here, but maybe there's a better way I don't see?)

Also, I can't remember/can't find what this + 1 in @path.bytesize + 1 for the count is for, it seems to work without it, soo... If there's no objection I'll remove it with the .to_slice fix soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I can't remember/can't find what this + 1 in @path.bytesize + 1 for the count is for, it seems to work without it

Looks like it was intended to copy the trailing NUL. You don't "need" it right now cause the call to Pointer#malloc above asks BoehmGC to allocate it, and BoehmGC zeros memory for you. Thus, you should either 1) Copy the trailing NUL 2) or set it manually. Not doing so makes that code rely on an implementation detail of the current GC implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @path.to_unsafe could be replaced with String#to_slice. I'm not sure if the returned slice contains the trailing NUL, please verify that.

Copy link
Contributor

@RX14 RX14 Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Papierkorb the returned slice doesn't contain the null - why would it? Slice is a crystal type and crystal doesn't use null terminated strings. The null is only there as a convenience to C.


sockaddr.as(LibC::Sockaddr*)
end
end
Expand Down
Loading