TcpSocket: Fix TlsConnection SocketHandle
Changes the StreamSocket::socket_handle() method to return a const ref.
Will be used for integrating with NetworkWaiter, following nearly-done
CL: https://chromium-review.googlesource.com/c/openscreen/+/1791373
The above change requires that all socket_handles is recieves are const
refs rather than a new object created for each call. Because the rest
of StreamSocketPosix is not thread safe, changing from an atomic_int
to a standard int in SocketHandlePosix should have no thread safety
issues.
See for further context:
https://chromium-review.googlesource.com/c/openscreen/+/1814978
(thought Chromium Gerrit isn't correctly setting up CL hierarchy, so
you will need to wade through extra files)
Change-Id: I0f9bc21ba2bdb3059188141a2875740d0e416d18
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/1816284
Reviewed-by: Max Yakimakha <yakimakha@chromium.org>
Reviewed-by: Jordan Bayles <jophba@chromium.org>
Commit-Queue: Jordan Bayles <jophba@chromium.org>
diff --git a/platform/impl/stream_socket.h b/platform/impl/stream_socket.h
index 3fcade4..6128915 100644
--- a/platform/impl/stream_socket.h
+++ b/platform/impl/stream_socket.h
@@ -25,8 +25,12 @@
// asynchronous.
class StreamSocket {
public:
+ StreamSocket() = default;
+ StreamSocket(const StreamSocket& other) = delete;
virtual ~StreamSocket() = default;
+ StreamSocket& operator=(const StreamSocket& other) = delete;
+
// Used by passive/server sockets to accept connection requests
// from a client.
virtual ErrorOr<std::unique_ptr<StreamSocket>> Accept() = 0;
@@ -47,7 +51,7 @@
virtual Error Listen(int max_backlog_size) = 0;
// Returns the file descriptor (e.g. fd or HANDLE pointer) for this socket.
- virtual SocketHandle socket_handle() const = 0;
+ virtual const SocketHandle& socket_handle() const = 0;
// Returns the connected remote address, if socket is connected.
virtual absl::optional<IPEndpoint> remote_address() const = 0;
diff --git a/platform/impl/stream_socket_posix.cc b/platform/impl/stream_socket_posix.cc
index aed7a67..447ab3c 100644
--- a/platform/impl/stream_socket_posix.cc
+++ b/platform/impl/stream_socket_posix.cc
@@ -27,7 +27,7 @@
StreamSocketPosix::StreamSocketPosix(SocketAddressPosix local_address,
int file_descriptor)
- : file_descriptor_(file_descriptor),
+ : handle_(file_descriptor),
version_(local_address.version()),
local_address_(local_address) {}
@@ -51,9 +51,8 @@
SocketAddressPosix new_remote_address = local_address_.value();
socklen_t remote_address_size = new_remote_address.size();
const int new_file_descriptor =
- accept(file_descriptor_.load(), new_remote_address.address(),
- &remote_address_size);
- if (new_file_descriptor == -1) {
+ accept(handle_.fd, new_remote_address.address(), &remote_address_size);
+ if (new_file_descriptor == kUnsetHandleFd) {
return CloseOnError(Error::Code::kSocketAcceptFailure);
}
@@ -75,7 +74,7 @@
return CloseOnError(Error::Code::kSocketInvalidState);
}
- if (bind(file_descriptor_.load(), local_address_.value().address(),
+ if (bind(handle_.fd, local_address_.value().address(),
local_address_.value().size()) != 0) {
return CloseOnError(Error::Code::kSocketBindFailure);
}
@@ -94,11 +93,12 @@
return Error::Code::kSocketInvalidState;
}
- const int file_descriptor_to_close = file_descriptor_.exchange(-1);
+ const int file_descriptor_to_close = handle_.fd;
if (close(file_descriptor_to_close) != 0) {
last_error_code_ = Error::Code::kSocketInvalidState;
return Error::Code::kSocketInvalidState;
}
+ handle_.fd = kUnsetHandleFd;
return Error::None();
}
@@ -113,8 +113,7 @@
}
SocketAddressPosix address(remote_endpoint);
- if (connect(file_descriptor_.load(), address.address(), address.size()) !=
- 0) {
+ if (connect(handle_.fd, address.address(), address.size()) != 0) {
return CloseOnError(Error::Code::kSocketConnectFailure);
}
@@ -125,7 +124,7 @@
struct sockaddr address;
socklen_t size = sizeof(address);
- if (getsockname(file_descriptor_.load(), &address, &size) != 0) {
+ if (getsockname(handle_.fd, &address, &size) != 0) {
return CloseOnError(Error::Code::kSocketConnectFailure);
}
@@ -147,17 +146,13 @@
return ReportSocketClosedError();
}
- if (listen(file_descriptor_.load(), max_backlog_size) != 0) {
+ if (listen(handle_.fd, max_backlog_size) != 0) {
return CloseOnError(Error::Code::kSocketListenFailure);
}
return Error::None();
}
-SocketHandle StreamSocketPosix::socket_handle() const {
- return SocketHandle{file_descriptor_.load()};
-}
-
absl::optional<IPEndpoint> StreamSocketPosix::remote_address() const {
if ((state_ != SocketState::kConnected) || !remote_address_) {
return absl::nullopt;
@@ -203,19 +198,20 @@
break;
}
- file_descriptor_ = socket(domain, SOCK_STREAM, 0);
- if (file_descriptor_ == -1) {
+ const int file_descriptor = socket(domain, SOCK_STREAM, 0);
+ if (file_descriptor == kUnsetHandleFd) {
last_error_code_ = Error::Code::kSocketInvalidState;
return Error::Code::kSocketInvalidState;
}
- const int current_flags = fcntl(file_descriptor_, F_GETFL, 0);
- if (fcntl(file_descriptor_, F_SETFL, current_flags | O_NONBLOCK) == -1) {
- close(file_descriptor_);
+ const int current_flags = fcntl(file_descriptor, F_GETFL, 0);
+ if (fcntl(file_descriptor, F_SETFL, current_flags | O_NONBLOCK) == -1) {
+ close(file_descriptor);
last_error_code_ = Error::Code::kSocketInvalidState;
return Error::Code::kSocketInvalidState;
}
+ handle_.fd = file_descriptor;
is_initialized_ = true;
// last_error_code_ should still be Error::None().
return Error::None();
diff --git a/platform/impl/stream_socket_posix.h b/platform/impl/stream_socket_posix.h
index b7eaa12..4502bce 100644
--- a/platform/impl/stream_socket_posix.h
+++ b/platform/impl/stream_socket_posix.h
@@ -40,7 +40,7 @@
Error Listen(int max_backlog_size) override;
// StreamSocket getter overrides.
- SocketHandle socket_handle() const override;
+ const SocketHandle& socket_handle() const override { return handle_; }
absl::optional<IPEndpoint> remote_address() const override;
absl::optional<IPEndpoint> local_address() const override;
SocketState state() const override;
@@ -56,7 +56,11 @@
Error CloseOnError(Error::Code error_code);
Error ReportSocketClosedError();
- std::atomic_int file_descriptor_ = {-1};
+ constexpr static int kUnsetHandleFd = -1;
+
+ // This SocketHandle object is expected to persist for the lieftime of this
+ // object. The internal fd may change, but the object may not be destroyed.
+ SocketHandle handle_{kUnsetHandleFd};
// last_error_code_ is an Error::Code instead of an Error so it meets
// atomic's (trivially) copyable and moveable requirements.