-
-
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
[cancelled, not using Crystal anymore] Add abstract unix socket handling #4056
Changes from 5 commits
2fc5863
66bd0ef
a02c69e
54d6dda
a802ee9
e93d6d7
277770b
5f2f223
7cddb29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
Socket::UNIXAddress.new("@/abstract.sock") | ||
end | ||
end | ||
|
||
it "is not abstract on non-Linux" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason this example should not run on linux target? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) } | ||
|
@@ -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 | ||
|
@@ -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 %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may get rid of the outer |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe also doublecheck that |
||
end | ||
end | ||
{% end %} | ||
|
||
it "deletes socket file on close" do | ||
path = "/tmp/crystal-test-unix-sock" | ||
|
||
|
@@ -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) | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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" | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]? == '@' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I didn't want to do that because unconsciously for me 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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Based on that, you made me think that it would be even more correct to check for equality with |
||
{% 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 | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if @abstract | ||
destination[0] = 0_u8 | ||
destination += 1 | ||
end | ||
destination.copy_from(@path.to_unsafe, @path.bytesize + 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is the reason you should prefer Slice: Your write to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @Papierkorb ! Also, I can't remember/can't find what this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Looks like it was intended to copy the trailing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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.
Don't need a regexp here, using
"not supported"
as argument will match if the string is included in the error message.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.
right, I though using a
String
would make it a exact match check. Stdlib is well done, it's an inclusion check, thanks 😄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's at least one other place in this file where a
Regex
is used instead of aString
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 ^^)crystal/spec/std/socket_spec.cr
Line 160 in 1978c8b