Change in dart/sdk[master]: [dart:io] Fix .address not being the local address for client side so...
127 views
Skip to first unread message
Jonas Termansen (Gerrit)
Dec 6, 2019, 11:08:08 AM12/6/19
to Lasse R.H. Nielsen, Zichang Guo, rev...@dartlang.org, vm-...@dartlang.org
Jonas Termansen would like Lasse R.H. Nielsen and Zichang Guo to review this change.
[dart:io] Fix .address not being the local address for client side sockets.
This is breaking change #TBA.
The .address getter for Socket, RawSocket, and RawSynchronousSocket used to
return the remote address for client side connections instead of the local
address (which there otherwise is no way to get). Inconsistently connections
accepted on the server side would provide the local address in the .address
getter.
This change fixes the .address getter so it always returns the local
address, which is a breaking change in behavior. However, code that wants
the remote address in this case has always been able to use .remoteAddress
for that purpose.
Additionally this change clears up confusion about the local and remote
address in the socket patch files, which fixes a 7 year old bug (#12693)
where a socket failure would provide the remote address but the local
port (leading to much confusion).
A new VM call is added to get the full local socket name instead of only its
port, matching the call to get the remote socket name.
Fixes https://github.com/dart-lang/sdk/issues/39674
Fixes https://github.com/dart-lang/sdk/issues/12693
Change-Id: I2141f9efa8cd1087b0903b1c2aa5de52f2c759cb
---
M CHANGELOG.md
M runtime/bin/io_natives.cc
M runtime/bin/socket.cc
M runtime/bin/socket_base.cc
M runtime/bin/socket_base.h
M runtime/bin/socket_base_fuchsia.cc
M runtime/bin/socket_base_linux.cc
M runtime/bin/socket_base_macos.cc
M runtime/bin/socket_base_win.cc
M runtime/bin/sync_socket.cc
M runtime/bin/sync_socket.h
M runtime/bin/sync_socket_android.cc
M runtime/bin/sync_socket_fuchsia.cc
M runtime/bin/sync_socket_linux.cc
M runtime/bin/sync_socket_macos.cc
M runtime/bin/sync_socket_win.cc
M sdk/lib/_internal/vm/bin/socket_patch.dart
M sdk/lib/_internal/vm/bin/sync_socket_patch.dart
M sdk/lib/io/socket.dart
M sdk/lib/io/sync_socket.dart
M sdk_nnbd/lib/_internal/vm/bin/socket_patch.dart
M sdk_nnbd/lib/_internal/vm/bin/sync_socket_patch.dart
M sdk_nnbd/lib/io/socket.dart
M sdk_nnbd/lib/io/sync_socket.dart
A tests/standalone_2/io/socket_address_test.dart
25 files changed, 316 insertions(+), 132 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 99a97c6..124e793 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -38,6 +38,18 @@
* **Breaking change**: Added `IOOverrides.serverSocketBind` to aid in writing
tests that wish to mock `ServerSocket.bind`.
+* **Breaking change** [#TBA](https://github.com/dart-lang/sdk/issues/TBA):
+ The `address` getter on the `Socket`, `RawSocket`, and `RawSynchronousSocket`
+ classes now provides the socket's local address for client connections.
+ Previously for client connections it would report the socket's remote address,
+ which is also available in the `remoteAddress` getter. Sockets accepted on the
+ server side always provided the correct local address in the `address` getter
+ and the correct remote address in the `remoteAddress` getter.
+
+ Code that relies on `address` actually being `remoteAddress` on the client
+ side needs to be updated to use `remoteAddress` instead, which is backwards
+ compatible.
+
### Dart VM
* New fields added to existing instances by a reload will now be initialized
diff --git a/runtime/bin/io_natives.cc b/runtime/bin/io_natives.cc
index f34671c..593c12b 100644
--- a/runtime/bin/io_natives.cc
+++ b/runtime/bin/io_natives.cc
@@ -135,7 +135,7 @@
V(Socket_CreateBindConnect, 5) \
V(Socket_CreateBindDatagram, 6) \
V(Socket_CreateConnect, 4) \
- V(Socket_GetPort, 1) \
+ V(Socket_GetSocketName, 1) \
V(Socket_GetRemotePeer, 1) \
V(Socket_GetError, 1) \
V(Socket_GetOption, 3) \
@@ -164,7 +164,7 @@
V(SynchronousSocket_Available, 1) \
V(SynchronousSocket_CloseSync, 1) \
V(SynchronousSocket_CreateConnectSync, 3) \
- V(SynchronousSocket_GetPort, 1) \
+ V(SynchronousSocket_GetSocketName, 1) \
V(SynchronousSocket_GetRemotePeer, 1) \
V(SynchronousSocket_LookupRequest, 2) \
V(SynchronousSocket_ShutdownRead, 1) \
diff --git a/runtime/bin/socket.cc b/runtime/bin/socket.cc
index 9db41a0..c5cab92 100644
--- a/runtime/bin/socket.cc
+++ b/runtime/bin/socket.cc
@@ -539,13 +539,26 @@
}
}
-void FUNCTION_NAME(Socket_GetPort)(Dart_NativeArguments args) {
+void FUNCTION_NAME(Socket_GetSocketName)(Dart_NativeArguments args) {
Socket* socket =
Socket::GetSocketIdNativeField(Dart_GetNativeArgument(args, 0));
OSError os_error;
- intptr_t port = SocketBase::GetPort(socket->fd());
- if (port > 0) {
- Dart_SetIntegerReturnValue(args, port);
+ intptr_t port = 0;
+ SocketAddress* addr = SocketBase::GetSocketName(socket->fd(), &port);
+ if (addr != NULL) {
+ Dart_Handle list = Dart_NewList(2);
+
+ Dart_Handle entry = Dart_NewList(3);
+ Dart_ListSetAt(entry, 0, Dart_NewInteger(addr->GetType()));
+ Dart_ListSetAt(entry, 1, Dart_NewStringFromCString(addr->as_string()));
+
+ RawAddr raw = addr->addr();
+ Dart_ListSetAt(entry, 2, SocketAddress::ToTypedData(raw));
+
+ Dart_ListSetAt(list, 0, entry);
+ Dart_ListSetAt(list, 1, Dart_NewInteger(port));
+ Dart_SetReturnValue(args, list);
+ delete addr;
} else {
Dart_SetReturnValue(args, DartUtils::NewDartOSError());
}
diff --git a/runtime/bin/socket_base.cc b/runtime/bin/socket_base.cc
index 8789c43..9b90714 100644
--- a/runtime/bin/socket_base.cc
+++ b/runtime/bin/socket_base.cc
@@ -155,6 +155,16 @@
}
}
+intptr_t SocketBase::GetPort(intptr_t fd) {
+ intptr_t port;
+ SocketAddress* address = SocketBase::GetSocketName(fd, &port);
+ if (address == nullptr) {
+ return 0;
+ }
+ delete address;
+ return port;
+}
+
void FUNCTION_NAME(InternetAddress_Parse)(Dart_NativeArguments args) {
const char* address =
DartUtils::GetStringValue(Dart_GetNativeArgument(args, 0));
diff --git a/runtime/bin/socket_base.h b/runtime/bin/socket_base.h
index 404357b..4d4e88d 100644
--- a/runtime/bin/socket_base.h
+++ b/runtime/bin/socket_base.h
@@ -171,6 +171,7 @@
// to bind the socket to a specific IP.
static bool IsBindError(intptr_t error_number);
static intptr_t GetPort(intptr_t fd);
+ static SocketAddress* GetSocketName(intptr_t fd, intptr_t* port);
static SocketAddress* GetRemotePeer(intptr_t fd, intptr_t* port);
static void GetError(intptr_t fd, OSError* os_error);
static int GetType(intptr_t fd);
diff --git a/runtime/bin/socket_base_fuchsia.cc b/runtime/bin/socket_base_fuchsia.cc
index ba90aa9..63cc8aa 100644
--- a/runtime/bin/socket_base_fuchsia.cc
+++ b/runtime/bin/socket_base_fuchsia.cc
@@ -166,16 +166,18 @@
return -1;
}
-intptr_t SocketBase::GetPort(intptr_t fd) {
+SocketAddress* SocketBase::GetSocketName(intptr_t fd, intptr_t* port) {
IOHandle* handle = reinterpret_cast<IOHandle*>(fd);
ASSERT(handle->fd() >= 0);
RawAddr raw;
socklen_t size = sizeof(raw);
- LOG_INFO("SocketBase::GetPort: calling getsockname(%ld)\n", handle->fd());
+ LOG_INFO("SocketBase::GetSocketName: calling getsockname(%ld)\n",
+ handle->fd());
if (NO_RETRY_EXPECTED(getsockname(handle->fd(), &raw.addr, &size))) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/socket_base_linux.cc b/runtime/bin/socket_base_linux.cc
index 9a52d96..ad38f89 100644
--- a/runtime/bin/socket_base_linux.cc
+++ b/runtime/bin/socket_base_linux.cc
@@ -123,14 +123,15 @@
return written_bytes;
}
-intptr_t SocketBase::GetPort(intptr_t fd) {
+SocketAddress* SocketBase::GetSocketName(intptr_t fd, intptr_t* port) {
ASSERT(fd >= 0);
RawAddr raw;
socklen_t size = sizeof(raw);
if (NO_RETRY_EXPECTED(getsockname(fd, &raw.addr, &size))) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/socket_base_macos.cc b/runtime/bin/socket_base_macos.cc
index a448911..f5bb570 100644
--- a/runtime/bin/socket_base_macos.cc
+++ b/runtime/bin/socket_base_macos.cc
@@ -122,14 +122,15 @@
return written_bytes;
}
-intptr_t SocketBase::GetPort(intptr_t fd) {
+SocketAddress* SocketBase::GetSocketName(intptr_t fd, intptr_t* port) {
ASSERT(fd >= 0);
RawAddr raw;
socklen_t size = sizeof(raw);
if (NO_RETRY_EXPECTED(getsockname(fd, &raw.addr, &size))) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/socket_base_win.cc b/runtime/bin/socket_base_win.cc
index 6aaef24..e7cf7d0 100644
--- a/runtime/bin/socket_base_win.cc
+++ b/runtime/bin/socket_base_win.cc
@@ -107,15 +107,19 @@
SocketAddress::GetAddrLength(addr));
}
-intptr_t SocketBase::GetPort(intptr_t fd) {
+SocketAddress* SocketBase::GetSocketName(intptr_t fd, intptr_t* port) {
ASSERT(reinterpret_cast<Handle*>(fd)->is_socket());
SocketHandle* socket_handle = reinterpret_cast<SocketHandle*>(fd);
RawAddr raw;
socklen_t size = sizeof(raw);
if (getsockname(socket_handle->socket(), &raw.addr, &size) == SOCKET_ERROR) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ // Clear the port before calling WSAAddressToString as WSAAddressToString
+ // includes the port in the formatted string.
+ SocketAddress::SetAddrPort(&raw, 0);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket.cc b/runtime/bin/sync_socket.cc
index 99cf792..68d1bc0 100644
--- a/runtime/bin/sync_socket.cc
+++ b/runtime/bin/sync_socket.cc
@@ -262,18 +262,41 @@
SynchronousSocket::ShutdownWrite(socket->fd());
}
-void FUNCTION_NAME(SynchronousSocket_GetPort)(Dart_NativeArguments args) {
+void FUNCTION_NAME(SynchronousSocket_GetSocketName)(Dart_NativeArguments args) {
SynchronousSocket* socket = NULL;
Dart_Handle result = SynchronousSocket::GetSocketIdNativeField(
Dart_GetNativeArgument(args, 0), &socket);
DART_CHECK_ERROR(result);
- intptr_t port = SynchronousSocket::GetPort(socket->fd());
- if (port > 0) {
- Dart_SetReturnValue(args, Dart_NewInteger(port));
- } else {
+ intptr_t port = 0;
+ SocketAddress* addr = SynchronousSocket::GetSocketName(socket->fd(), &port);
+ if (addr == NULL) {
Dart_SetReturnValue(args, DartUtils::NewDartOSError());
+ return;
}
+ Dart_Handle list = Dart_NewList(2);
+ DART_CHECK_ERROR_AND_CLEANUP(list, addr);
+
+ Dart_Handle entry = Dart_NewList(3);
+ DART_CHECK_ERROR_AND_CLEANUP(entry, addr);
+
+ Dart_Handle error =
+ Dart_ListSetAt(entry, 0, Dart_NewInteger(addr->GetType()));
+ DART_CHECK_ERROR_AND_CLEANUP(error, addr);
+ error =
+ Dart_ListSetAt(entry, 1, Dart_NewStringFromCString(addr->as_string()));
+ DART_CHECK_ERROR_AND_CLEANUP(error, addr);
+
+ RawAddr raw = addr->addr();
+ error = Dart_ListSetAt(entry, 2, SocketAddress::ToTypedData(raw));
+ DART_CHECK_ERROR_AND_CLEANUP(error, addr);
+
+ error = Dart_ListSetAt(list, 0, entry);
+ DART_CHECK_ERROR_AND_CLEANUP(error, addr);
+ error = Dart_ListSetAt(list, 1, Dart_NewInteger(port));
+ DART_CHECK_ERROR_AND_CLEANUP(error, addr);
+ Dart_SetReturnValue(args, list);
+ delete addr;
}
void FUNCTION_NAME(SynchronousSocket_GetRemotePeer)(Dart_NativeArguments args) {
diff --git a/runtime/bin/sync_socket.h b/runtime/bin/sync_socket.h
index 85f3554..9e47e50 100644
--- a/runtime/bin/sync_socket.h
+++ b/runtime/bin/sync_socket.h
@@ -29,7 +29,7 @@
SynchronousSocket** socket);
static intptr_t Available(intptr_t fd);
- static intptr_t GetPort(intptr_t fd);
+ static SocketAddress* GetSocketName(intptr_t fd, intptr_t* port);
static SocketAddress* GetRemotePeer(intptr_t fd, intptr_t* port);
static intptr_t Read(intptr_t fd, void* buffer, intptr_t num_bytes);
static intptr_t Write(intptr_t fd, const void* buffer, intptr_t num_bytes);
diff --git a/runtime/bin/sync_socket_android.cc b/runtime/bin/sync_socket_android.cc
index b051e3a..82d9309 100644
--- a/runtime/bin/sync_socket_android.cc
+++ b/runtime/bin/sync_socket_android.cc
@@ -54,8 +54,8 @@
return SocketBase::Available(fd);
}
-intptr_t SynchronousSocket::GetPort(intptr_t fd) {
- return SocketBase::GetPort(fd);
+SocketAddress* SynchronousSocket::GetSocketName(intptr_t fd, intptr_t* port) {
+ return SocketBase::GetSocketName(fd, port);
}
SocketAddress* SynchronousSocket::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket_fuchsia.cc b/runtime/bin/sync_socket_fuchsia.cc
index 3a6fe0d..84d2277 100644
--- a/runtime/bin/sync_socket_fuchsia.cc
+++ b/runtime/bin/sync_socket_fuchsia.cc
@@ -54,8 +54,8 @@
return SocketBase::Available(fd);
}
-intptr_t SynchronousSocket::GetPort(intptr_t fd) {
- return SocketBase::GetPort(fd);
+SocketAddress* SynchronousSocket::GetSocketName(intptr_t fd, intptr_t* port) {
+ return SocketBase::GetSocketName(fd, port);
}
SocketAddress* SynchronousSocket::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket_linux.cc b/runtime/bin/sync_socket_linux.cc
index 1fb564b..f7caa41 100644
--- a/runtime/bin/sync_socket_linux.cc
+++ b/runtime/bin/sync_socket_linux.cc
@@ -54,8 +54,8 @@
return SocketBase::Available(fd);
}
-intptr_t SynchronousSocket::GetPort(intptr_t fd) {
- return SocketBase::GetPort(fd);
+SocketAddress* SynchronousSocket::GetSocketName(intptr_t fd, intptr_t* port) {
+ return SocketBase::GetSocketName(fd, port);
}
SocketAddress* SynchronousSocket::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket_macos.cc b/runtime/bin/sync_socket_macos.cc
index 5ee9b68..abffaee 100644
--- a/runtime/bin/sync_socket_macos.cc
+++ b/runtime/bin/sync_socket_macos.cc
@@ -57,8 +57,8 @@
return SocketBase::Available(fd);
}
-intptr_t SynchronousSocket::GetPort(intptr_t fd) {
- return SocketBase::GetPort(fd);
+SocketAddress* SynchronousSocket::GetSocketName(intptr_t fd, intptr_t* port) {
+ return SocketBase::GetSocketName(fd, port);
}
SocketAddress* SynchronousSocket::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket_win.cc b/runtime/bin/sync_socket_win.cc
index d4e72fe..09eff46 100644
--- a/runtime/bin/sync_socket_win.cc
+++ b/runtime/bin/sync_socket_win.cc
@@ -40,14 +40,18 @@
return (result == SOCKET_ERROR) ? -1 : static_cast<intptr_t>(available);
}
-intptr_t SynchronousSocket::GetPort(intptr_t fd) {
+SocketAddress* SynchronousSocket::GetSocketName(intptr_t fd, intptr_t* port) {
SOCKET socket = static_cast<SOCKET>(fd);
RawAddr raw;
socklen_t size = sizeof(raw);
if (getsockname(socket, &raw.addr, &size) == SOCKET_ERROR) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ // Clear the port before calling WSAAddressToString as WSAAddressToString
+ // includes the port in the formatted string.
+ SocketAddress::SetAddrPort(&raw, 0);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SynchronousSocket::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/sdk/lib/_internal/vm/bin/socket_patch.dart b/sdk/lib/_internal/vm/bin/socket_patch.dart
index e8e2bd1..363da72 100644
--- a/sdk/lib/_internal/vm/bin/socket_patch.dart
+++ b/sdk/lib/_internal/vm/bin/socket_patch.dart
@@ -358,11 +358,10 @@
// The type flags for this socket.
final int typeFlags;
- // Holds the port of the socket, 0 if not known.
- int localPort = 0;
-
- // Holds the address used to connect or bind the socket.
- InternetAddress localAddress;
+ // The requested remote address and port, for error messages in case the
+ // connection fails.
+ InternetAddress requestedRemoteAddress;
+ int requestedRemotePort;
int available = 0;
@@ -498,7 +497,8 @@
}
final _InternetAddress address = it.current;
var socket = new _NativeSocket.normal();
- socket.localAddress = address;
+ socket.requestedRemoteAddress = address;
+ socket.requestedRemotePort = port;
var result;
if (sourceAddress == null) {
result = socket.nativeCreateConnect(
@@ -637,14 +637,12 @@
final address = await _resolveHost(host);
var socket = new _NativeSocket.listen();
- socket.localAddress = address;
var result = socket.nativeCreateBindListen(
address._in_addr, port, backlog, v6Only, shared, address._scope_id);
if (result is OSError) {
throw new SocketException("Failed to create server socket",
osError: result, address: address, port: port);
}
- if (port != 0) socket.localPort = port;
setupResourceInfo(socket);
socket.connectToEventHandler();
return socket;
@@ -661,20 +659,18 @@
final address = await _resolveHost(host);
- var socket = new _NativeSocket.datagram(address);
+ var socket = new _NativeSocket.datagram();
var result = socket.nativeCreateBindDatagram(
address._in_addr, port, reuseAddress, reusePort, ttl);
if (result is OSError) {
throw new SocketException("Failed to create datagram socket",
osError: result, address: address, port: port);
}
- if (port != 0) socket.localPort = port;
setupResourceInfo(socket);
return socket;
}
- _NativeSocket.datagram(this.localAddress)
- : typeFlags = typeNormalSocket | typeUdpSocket;
+ _NativeSocket.datagram() : typeFlags = typeNormalSocket | typeUdpSocket;
_NativeSocket.normal() : typeFlags = typeNormalSocket | typeTcpSocket;
@@ -857,8 +853,6 @@
returnTokens(listeningTokenBatchSize);
var socket = new _NativeSocket.normal();
if (nativeAccept(socket) != true) return null;
- socket.localPort = localPort;
- socket.localAddress = address;
setupResourceInfo(socket);
// TODO(ricow): Remove when we track internal and pipe uses.
assert(resourceInfo != null || isPipe || isInternal || isInternalSignal);
@@ -870,11 +864,10 @@
}
int get port {
- if (localPort != 0) return localPort;
if (isClosing || isClosed) throw const SocketException.closed();
- var result = nativeGetPort();
+ var result = nativeGetSocketName();
if (result is OSError) throw result;
- return localPort = result;
+ return result[1];
}
int get remotePort {
@@ -884,7 +877,14 @@
return result[1];
}
- InternetAddress get address => localAddress;
+ InternetAddress get address {
+ if (isClosing || isClosed) throw const SocketException.closed();
+ var result = nativeGetSocketName();
+ if (result is OSError) throw result;
+ var addr = result[0];
+ var type = new InternetAddressType._from(addr[0]);
+ return new _InternetAddress(addr[1], null, addr[2]);
+ }
InternetAddress get remoteAddress {
if (isClosing || isClosed) throw const SocketException.closed();
@@ -1154,7 +1154,19 @@
}
void reportError(error, StackTrace st, String message) {
- var e = createError(error, message, address, localPort);
+ InternetAddress errorAddress;
+ int errorPort;
+ try {
+ // These will throw an OSError ENOTCONN if the connection fails.
+ errorAddress = remoteAddress;
+ errorPort = port;
+ } on OSError catch (_) {
+ // Fall back on the requested remote information if we can't get the
+ // authoritative information.
+ errorAddress = requestedRemoteAddress;
+ errorPort = requestedRemotePort;
+ }
+ var e = createError(error, message, errorAddress, errorPort);
// Invoke the error handler if any.
if (eventHandlers[errorEvent] != null) {
eventHandlers[errorEvent](e, st);
@@ -1252,8 +1264,8 @@
nativeCreateBindDatagram(Uint8List addr, int port, bool reuseAddress,
bool reusePort, int ttl) native "Socket_CreateBindDatagram";
nativeAccept(_NativeSocket socket) native "ServerSocket_Accept";
- int nativeGetPort() native "Socket_GetPort";
- List nativeGetRemotePeer() native "Socket_GetRemotePeer";
+ nativeGetSocketName() native "Socket_GetSocketName";
+ nativeGetRemotePeer() native "Socket_GetRemotePeer";
int nativeGetSocketId() native "Socket_GetSocketId";
OSError nativeGetError() native "Socket_GetError";
nativeGetOption(int option, int protocol) native "Socket_GetOption";
diff --git a/sdk/lib/_internal/vm/bin/sync_socket_patch.dart b/sdk/lib/_internal/vm/bin/sync_socket_patch.dart
index d98649e..7a9a09b 100644
--- a/sdk/lib/_internal/vm/bin/sync_socket_patch.dart
+++ b/sdk/lib/_internal/vm/bin/sync_socket_patch.dart
@@ -56,12 +56,6 @@
bool isClosedRead = false;
bool isClosedWrite = false;
- // Holds the address used to connect the socket.
- InternetAddress localAddress;
-
- // Holds the port of the socket, 0 if not known.
- int localPort = 0;
-
_ReadWriteResourceInfo resourceInfo;
static _NativeSynchronousSocket connectSync(host, int port) {
@@ -92,7 +86,6 @@
}
var address = it.current;
var socket = new _NativeSynchronousSocket();
- socket.localAddress = address;
var result = socket._nativeCreateConnectSync(address._in_addr, port);
if (result is OSError) {
// Keep first error, if present.
@@ -118,21 +111,29 @@
return connectNext();
}
- InternetAddress get address => localAddress;
int get available => _nativeAvailable();
- int get port {
- if (localPort != 0) {
- return localPort;
- }
+ InternetAddress get address {
if (isClosed) {
throw const SocketException.closed();
}
- var result = _nativeGetPort();
+ var result = _nativeGetSocketName();
if (result is OSError) {
throw result;
}
- return localPort = result;
+ var addr = result[0];
+ return new _InternetAddress(addr[1], null, addr[2]);
+ }
+
+ int get port {
+ if (isClosed) {
+ throw const SocketException.closed();
+ }
+ var result = _nativeGetSocketName();
+ if (result is OSError) {
+ throw result;
+ }
+ return result[1];
}
InternetAddress get remoteAddress {
@@ -339,8 +340,8 @@
native "SynchronousSocket_CreateConnectSync";
_nativeAvailable() native "SynchronousSocket_Available";
_nativeCloseSync() native "SynchronousSocket_CloseSync";
- int _nativeGetPort() native "SynchronousSocket_GetPort";
- List _nativeGetRemotePeer() native "SynchronousSocket_GetRemotePeer";
+ _nativeGetSocketName() native "SynchronousSocket_GetSocketName";
+ _nativeGetRemotePeer() native "SynchronousSocket_GetRemotePeer";
_nativeRead(int len) native "SynchronousSocket_Read";
_nativeReadInto(List<int> buffer, int offset, int bytes)
native "SynchronousSocket_ReadList";
diff --git a/sdk/lib/io/socket.dart b/sdk/lib/io/socket.dart
index 17f9e05..47f4f9f 100644
--- a/sdk/lib/io/socket.dart
+++ b/sdk/lib/io/socket.dart
@@ -651,22 +651,26 @@
int write(List<int> buffer, [int offset, int count]);
/**
- * Returns the port used by this socket.
+ * The local port used by this socket.
*/
int get port;
/**
- * Returns the remote port connected to by this socket.
+ * The remote port connected to by this socket.
*/
int get remotePort;
/**
- * Returns the [InternetAddress] used to connect this socket.
+ * The local address used by this socket.
+ *
+ * Note: Prior to Dart 2.8, client side sockets would return the remote
+ * address instead of the local address. Sockets accepted on the server side
+ * always returned the local address.
*/
InternetAddress get address;
/**
- * Returns the remote [InternetAddress] connected to by this socket.
+ * The remote address connected to by this socket.
*/
InternetAddress get remoteAddress;
@@ -813,22 +817,26 @@
void setRawOption(RawSocketOption option);
/**
- * Returns the port used by this socket.
+ * The local port used by this socket.
*/
int get port;
/**
- * Returns the remote port connected to by this socket.
+ * The remote port connected to by this socket.
*/
int get remotePort;
/**
- * Returns the [InternetAddress] used to connect this socket.
+ * The local address used by this socket.
+ *
+ * Note: Prior to Dart 2.8, client side sockets would return the remote
+ * address instead of the local address. Sockets accepted on the server side
+ * always returned the local address.
*/
InternetAddress get address;
/**
- * Returns the remote [InternetAddress] connected to by this socket.
+ * The remote address connected to by this socket.
*/
InternetAddress get remoteAddress;
diff --git a/sdk/lib/io/sync_socket.dart b/sdk/lib/io/sync_socket.dart
index 386dc2e..9c15403 100644
--- a/sdk/lib/io/sync_socket.dart
+++ b/sdk/lib/io/sync_socket.dart
@@ -90,7 +90,7 @@
void writeFromSync(List<int> buffer, [int start = 0, int end]);
/**
- * The port used by this socket.
+ * The local port used by this socket.
*/
int get port;
@@ -100,12 +100,16 @@
int get remotePort;
/**
- * The [InternetAddress] used to connect this socket.
+ * The local address used by this socket.
+ *
+ * Note: Prior to Dart 2.8, client side sockets would return the remote
+ * address instead of the local address. Sockets accepted on the server side
+ * always returned the local address.
*/
InternetAddress get address;
/**
- * The remote [InternetAddress] connected to by this socket.
+ * The remote address connected to by this socket.
*/
InternetAddress get remoteAddress;
}
diff --git a/sdk_nnbd/lib/_internal/vm/bin/socket_patch.dart b/sdk_nnbd/lib/_internal/vm/bin/socket_patch.dart
index 84baa46..738a4cf 100644
--- a/sdk_nnbd/lib/_internal/vm/bin/socket_patch.dart
+++ b/sdk_nnbd/lib/_internal/vm/bin/socket_patch.dart
@@ -358,11 +358,10 @@
// The type flags for this socket.
final int typeFlags;
- // Holds the port of the socket, 0 if not known.
- int localPort = 0;
-
- // Holds the address used to connect or bind the socket.
- InternetAddress localAddress;
+ // The requested remote address and port, for error messages in case the
+ // connection fails.
+ InternetAddress requestedRemoteAddress;
+ int requestedRemotePort;
int available = 0;
@@ -498,7 +497,8 @@
}
final _InternetAddress address = it.current;
var socket = new _NativeSocket.normal();
- socket.localAddress = address;
+ socket.requestedRemoteAddress = address;
+ socket.requestedRemotePort = port;
var result;
if (sourceAddress == null) {
result = socket.nativeCreateConnect(
@@ -637,14 +637,12 @@
final address = await _resolveHost(host);
var socket = new _NativeSocket.listen();
- socket.localAddress = address;
var result = socket.nativeCreateBindListen(
address._in_addr, port, backlog, v6Only, shared, address._scope_id);
if (result is OSError) {
throw new SocketException("Failed to create server socket",
osError: result, address: address, port: port);
}
- if (port != 0) socket.localPort = port;
setupResourceInfo(socket);
socket.connectToEventHandler();
return socket;
@@ -661,20 +659,18 @@
final address = await _resolveHost(host);
- var socket = new _NativeSocket.datagram(address);
+ var socket = new _NativeSocket.datagram();
var result = socket.nativeCreateBindDatagram(
address._in_addr, port, reuseAddress, reusePort, ttl);
if (result is OSError) {
throw new SocketException("Failed to create datagram socket",
osError: result, address: address, port: port);
}
- if (port != 0) socket.localPort = port;
setupResourceInfo(socket);
return socket;
}
- _NativeSocket.datagram(this.localAddress)
- : typeFlags = typeNormalSocket | typeUdpSocket;
+ _NativeSocket.datagram() : typeFlags = typeNormalSocket | typeUdpSocket;
_NativeSocket.normal() : typeFlags = typeNormalSocket | typeTcpSocket;
@@ -857,8 +853,6 @@
returnTokens(listeningTokenBatchSize);
var socket = new _NativeSocket.normal();
if (nativeAccept(socket) != true) return null;
- socket.localPort = localPort;
- socket.localAddress = address;
setupResourceInfo(socket);
// TODO(ricow): Remove when we track internal and pipe uses.
assert(resourceInfo != null || isPipe || isInternal || isInternalSignal);
@@ -870,11 +864,10 @@
}
int get port {
- if (localPort != 0) return localPort;
if (isClosing || isClosed) throw const SocketException.closed();
- var result = nativeGetPort();
+ var result = nativeGetSocketName();
if (result is OSError) throw result;
- return localPort = result;
+ return result[1];
}
int get remotePort {
@@ -884,7 +877,14 @@
return result[1];
}
- InternetAddress get address => localAddress;
+ InternetAddress get address {
+ if (isClosing || isClosed) throw const SocketException.closed();
+ var result = nativeGetSocketName();
+ if (result is OSError) throw result;
+ var addr = result[0];
+ var type = new InternetAddressType._from(addr[0]);
+ return new _InternetAddress(addr[1], null, addr[2]);
+ }
InternetAddress get remoteAddress {
if (isClosing || isClosed) throw const SocketException.closed();
@@ -1154,7 +1154,19 @@
}
void reportError(error, StackTrace st, String message) {
- var e = createError(error, message, address, localPort);
+ InternetAddress errorAddress;
+ int errorPort;
+ try {
+ // These will throw an OSError ENOTCONN if the connection fails.
+ errorAddress = remoteAddress;
+ errorPort = port;
+ } on OSError catch (_) {
+ // Fall back on the requested remote information if we can't get the
+ // authoritative information.
+ errorAddress = requestedRemoteAddress;
+ errorPort = requestedRemotePort;
+ }
+ var e = createError(error, message, errorAddress, errorPort);
// Invoke the error handler if any.
if (eventHandlers[errorEvent] != null) {
eventHandlers[errorEvent](e, st);
@@ -1252,8 +1264,8 @@
nativeCreateBindDatagram(Uint8List addr, int port, bool reuseAddress,
bool reusePort, int ttl) native "Socket_CreateBindDatagram";
nativeAccept(_NativeSocket socket) native "ServerSocket_Accept";
- int nativeGetPort() native "Socket_GetPort";
- List nativeGetRemotePeer() native "Socket_GetRemotePeer";
+ nativeGetSocketName() native "Socket_GetSocketName";
+ nativeGetRemotePeer() native "Socket_GetRemotePeer";
int nativeGetSocketId() native "Socket_GetSocketId";
OSError nativeGetError() native "Socket_GetError";
nativeGetOption(int option, int protocol) native "Socket_GetOption";
diff --git a/sdk_nnbd/lib/_internal/vm/bin/sync_socket_patch.dart b/sdk_nnbd/lib/_internal/vm/bin/sync_socket_patch.dart
index 941aba4..f7b8253 100644
--- a/sdk_nnbd/lib/_internal/vm/bin/sync_socket_patch.dart
+++ b/sdk_nnbd/lib/_internal/vm/bin/sync_socket_patch.dart
@@ -56,12 +56,6 @@
bool isClosedRead = false;
bool isClosedWrite = false;
- // Holds the address used to connect the socket.
- InternetAddress localAddress;
-
- // Holds the port of the socket, 0 if not known.
- int localPort = 0;
-
_ReadWriteResourceInfo resourceInfo;
static _NativeSynchronousSocket connectSync(host, int port) {
@@ -92,7 +86,6 @@
}
var address = it.current;
var socket = new _NativeSynchronousSocket();
- socket.localAddress = address;
var result = socket._nativeCreateConnectSync(address._in_addr, port);
if (result is OSError) {
// Keep first error, if present.
@@ -118,21 +111,29 @@
return connectNext();
}
- InternetAddress get address => localAddress;
int get available => _nativeAvailable();
- int get port {
- if (localPort != 0) {
- return localPort;
- }
+ InternetAddress get address {
if (isClosed) {
throw const SocketException.closed();
}
- var result = _nativeGetPort();
+ var result = _nativeGetSocketName();
if (result is OSError) {
throw result;
}
- return localPort = result;
+ var addr = result[0];
+ return new _InternetAddress(addr[1], null, addr[2]);
+ }
+
+ int get port {
+ if (isClosed) {
+ throw const SocketException.closed();
+ }
+ var result = _nativeGetSocketName();
+ if (result is OSError) {
+ throw result;
+ }
+ return result[1];
}
InternetAddress get remoteAddress {
@@ -339,8 +340,8 @@
native "SynchronousSocket_CreateConnectSync";
_nativeAvailable() native "SynchronousSocket_Available";
_nativeCloseSync() native "SynchronousSocket_CloseSync";
- int _nativeGetPort() native "SynchronousSocket_GetPort";
- List _nativeGetRemotePeer() native "SynchronousSocket_GetRemotePeer";
+ _nativeGetSocketName() native "SynchronousSocket_GetSocketName";
+ _nativeGetRemotePeer() native "SynchronousSocket_GetRemotePeer";
_nativeRead(int len) native "SynchronousSocket_Read";
_nativeReadInto(List<int> buffer, int offset, int bytes)
native "SynchronousSocket_ReadList";
diff --git a/sdk_nnbd/lib/io/socket.dart b/sdk_nnbd/lib/io/socket.dart
index 6df5273..e54cef4 100644
--- a/sdk_nnbd/lib/io/socket.dart
+++ b/sdk_nnbd/lib/io/socket.dart
@@ -650,22 +650,26 @@
int write(List<int> buffer, [int offset, int count]);
/**
- * Returns the port used by this socket.
+ * The local port used by this socket.
*/
int get port;
/**
- * Returns the remote port connected to by this socket.
+ * The remote port connected to by this socket.
*/
int get remotePort;
/**
- * Returns the [InternetAddress] used to connect this socket.
+ * The local address used by this socket.
+ *
+ * Note: Prior to Dart 2.8, client side sockets would return the remote
+ * address instead of the local address. Sockets accepted on the server side
+ * always returned the local address.
*/
InternetAddress get address;
/**
- * Returns the remote [InternetAddress] connected to by this socket.
+ * The remote address connected to by this socket.
*/
InternetAddress get remoteAddress;
@@ -812,22 +816,26 @@
void setRawOption(RawSocketOption option);
/**
- * Returns the port used by this socket.
+ * The local port used by this socket.
*/
int get port;
/**
- * Returns the remote port connected to by this socket.
+ * The remote port connected to by this socket.
*/
int get remotePort;
/**
- * Returns the [InternetAddress] used to connect this socket.
+ * The local address used by this socket.
+ *
+ * Note: Prior to Dart 2.8, client side sockets would return the remote
+ * address instead of the local address. Sockets accepted on the server side
+ * always returned the local address.
*/
InternetAddress get address;
/**
- * Returns the remote [InternetAddress] connected to by this socket.
+ * The remote address connected to by this socket.
*/
InternetAddress get remoteAddress;
diff --git a/sdk_nnbd/lib/io/sync_socket.dart b/sdk_nnbd/lib/io/sync_socket.dart
index 0b7d5b1..d016e97 100644
--- a/sdk_nnbd/lib/io/sync_socket.dart
+++ b/sdk_nnbd/lib/io/sync_socket.dart
@@ -90,7 +90,7 @@
void writeFromSync(List<int> buffer, [int start = 0, int end]);
/**
- * The port used by this socket.
+ * The local port used by this socket.
*/
int get port;
@@ -100,12 +100,16 @@
int get remotePort;
/**
- * The [InternetAddress] used to connect this socket.
+ * The local address used by this socket.
+ *
+ * Note: Prior to Dart 2.8, client side sockets would return the remote
+ * address instead of the local address. Sockets accepted on the server side
+ * always returned the local address.
*/
InternetAddress get address;
/**
- * The remote [InternetAddress] connected to by this socket.
+ * The remote address connected to by this socket.
*/
InternetAddress get remoteAddress;
}
diff --git a/tests/standalone_2/io/socket_address_test.dart b/tests/standalone_2/io/socket_address_test.dart
new file mode 100644
index 0000000..5519b3f
--- /dev/null
+++ b/tests/standalone_2/io/socket_address_test.dart
@@ -0,0 +1,63 @@
+// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// This test verifies the Socket implementation provides the right information
+// for the local address and port and the remote address and port.
+
+import "dart:async";
+import "dart:io";
+
+import "package:expect/expect.dart";
+
+Future main() async {
+ final server = await ServerSocket.bind("127.0.0.1", 0);
+ try {
+ // Find another local network address that can connect to 127.0.0.1 so we
+ // can verify if the local/remote address/port information is correct. Using
+ // 127.0.0.1 for both sides doesn't work because client connections used to
+ // report the remote address as the local address and we need to test that's
+ // no longer the case, so the addresses must be distinct.
+ if (!NetworkInterface.listSupported) return;
+ for (final networkInterface in await NetworkInterface.list()) {
+ for (final sourceAddress in networkInterface.addresses) {
+ if (sourceAddress.type != InternetAddressType.IPv4 ||
+ sourceAddress.isLinkLocal ||
+ sourceAddress.isLoopback ||
+ sourceAddress.isMulticast) {
+ continue;
+ }
+ final completer = new Completer<Socket>();
+ server
+ .listen((Socket serverSocket) => completer.complete(serverSocket));
+ final clientSocket = await Socket.connect("127.0.0.1", server.port,
+ sourceAddress: sourceAddress);
+ final serverSocket = await completer.future;
+ Expect.equals(serverSocket.address, new InternetAddress("127.0.0.1"),
+ "serverSocket.address == 127.0.0.1");
+ Expect.equals(clientSocket.address, sourceAddress,
+ "serverSocket.address == sourceAddress");
+ Expect.notEquals(clientSocket.address, serverSocket.address,
+ "clientSocket.address != serverSocket.address");
+ Expect.notEquals(clientSocket.remoteAddress, serverSocket.remoteAddress,
+ "clientSocket.remoteAddress != serverSocket.remoteAddress");
+ Expect.equals(clientSocket.address, serverSocket.remoteAddress,
+ "clientSocket.address == serverSocket.remoteAddress");
+ Expect.equals(clientSocket.remoteAddress, serverSocket.address,
+ "clientSocket.remoteAddress == serverSocket.address");
+ Expect.equals(clientSocket.port, serverSocket.remotePort,
+ "clientSocket.port == serverSocket.remotePort");
+ Expect.equals(clientSocket.remotePort, serverSocket.port,
+ "clientSocket.remotePort == serverSocket.port");
+ serverSocket.destroy();
+ clientSocket.destroy();
+ // These properties only needs to be verified once, so just stop here.
+ return;
+ }
+ }
+ // Just assume things are fine if we couldn't find a proper source address.
+ // That's the best we can do.
+ } finally {
+ server.close();
+ }
+}
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Jonas Termansen (Gerrit)
Dec 6, 2019, 11:08:08 AM12/6/19
to rev...@dartlang.org, vm-...@dartlang.org, Lasse R.H. Nielsen, Zichang Guo
Patch set 1:Commit-Queue +1
Jonas Termansen (Gerrit)
Dec 6, 2019, 11:38:46 AM12/6/19
to rev...@dartlang.org, vm-...@dartlang.org, commi...@chromium.org, Lasse R.H. Nielsen, Zichang Guo
Okay there's some test failures. Looks like some tests needs to be updated as the provided addresses are slightly different and maybe there's some bugs as well. I'll dive into those next week.
Zichang Guo (Gerrit)
Dec 6, 2019, 12:05:17 PM12/6/19
to Jonas Termansen, rev...@dartlang.org, vm-...@dartlang.org, commi...@chromium.org, Lasse R.H. Nielsen
This fix looks good to me. But I'm worried whether the fix will break users old codes. Similar to those failing tests, users might rely on this wrong "address".
Do you think it is better to add another "localAddress" getter? "address" will not be changed. I would expect this is less breaking to users.
Jonas Termansen (Gerrit)
Dec 6, 2019, 2:15:07 PM12/6/19
to rev...@dartlang.org, vm-...@dartlang.org, Zichang Guo, commi...@chromium.org, Lasse R.H. Nielsen
Patch Set 1:
This fix looks good to me. But I'm worried whether the fix will break users old codes. Similar to those failing tests, users might rely on this wrong "address".
Do you think it is better to add another "localAddress" getter? "address" will not be changed. I would expect this is less breaking to users.
Hi Zichang, your suggestion was my original approach. However, I did a search and failed to find any uses of .address, and .remoteAddress is there explicitly for those that want it. I'll do another full search of pub and internally before I proceed with this breaking change to get a better understanding. In any case, even if such breakage existed, it would likely be mild.
On the contrary, adding a localAddress would be probably much more breaking for all implementations of Socket and such. There should also be a matching localPort. It would also make 'address' on ServerSocket be inconsistent and other places too. I talked it over with Lasse a while back and we reached a tentative agreement that fixing 'address' would break very little and would get us closest to the ideal semantics. But I'll verify those assumptions of course.
Zichang Guo (Gerrit)
Dec 6, 2019, 5:46:11 PM12/6/19
to Jonas Termansen, rev...@dartlang.org, vm-...@dartlang.org, commi...@chromium.org, Lasse R.H. Nielsen
Patch Set 1:
Patch Set 1:
This fix looks good to me. But I'm worried whether the fix will break users old codes. Similar to those failing tests, users might rely on this wrong "address".
Do you think it is better to add another "localAddress" getter? "address" will not be changed. I would expect this is less breaking to users.Hi Zichang, your suggestion was my original approach. However, I did a search and failed to find any uses of .address, and .remoteAddress is there explicitly for those that want it. I'll do another full search of pub and internally before I proceed with this breaking change to get a better understanding. In any case, even if such breakage existed, it would likely be mild.
On the contrary, adding a localAddress would be probably much more breaking for all implementations of Socket and such. There should also be a matching localPort. It would also make 'address' on ServerSocket be inconsistent and other places too. I talked it over with Lasse a while back and we reached a tentative agreement that fixing 'address' would break very little and would get us closest to the ideal semantics. But I'll verify those assumptions of course.
Sounds good. Look forward to your investigation results.
Jonas Termansen (Gerrit)
Mar 11, 2021, 11:23:26 AM3/11/21
to Alexander Aprelev, rev...@dartlang.org, vm-...@dartlang.org, Lasse R.H. Nielsen
Attention is currently required from: Alexander Aprelev.
Jonas Termansen would like Alexander Aprelev to review this change.
[dart:io] Fix .address not being the local address for client sockets.
This is breaking change #TBA.
The .address getter for Socket, RawSocket, and RawSynchronousSocket used
to return the remote address for client side connections instead of the
local address (which there otherwise is no way to get). This was
inconsistent with connections accepted on the server side that would
provide the local address in the .address getter.
This change fixes the .address getter so it always returns the local
address, which is a breaking change in behavior. However, code that
wants the remote address in this case has always been able to use
.remoteAddress for that purpose.
Additionally this change clears up confusion about the local and remote
address in the socket patch files, which fixes a 9 year old bug (#12693)
where a socket failure would provide the remote address but the local
port (leading to much confusion).
A new VM call is added to get the full local socket name instead of only
its port, matching the call to get the remote socket name.
Fixes https://github.com/dart-lang/sdk/issues/39674
Fixes https://github.com/dart-lang/sdk/issues/12693
TEST=tests/standalone/io/socket_address_test.dart
Change-Id: I2141f9efa8cd1087b0903b1c2aa5de52f2c759cb
---
A BREAKING-CHANGE-REQUEST
M CHANGELOG.md
M runtime/bin/io_natives.cc
M runtime/bin/socket.cc
M runtime/bin/socket_base.cc
M runtime/bin/socket_base.h
M runtime/bin/socket_base_fuchsia.cc
M runtime/bin/socket_base_linux.cc
M runtime/bin/socket_base_macos.cc
M runtime/bin/socket_base_win.cc
M runtime/bin/sync_socket.cc
M runtime/bin/sync_socket.h
M runtime/bin/sync_socket_android.cc
M runtime/bin/sync_socket_fuchsia.cc
M runtime/bin/sync_socket_linux.cc
M runtime/bin/sync_socket_macos.cc
M runtime/bin/sync_socket_win.cc
M sdk/lib/_internal/vm/bin/socket_patch.dart
M sdk/lib/_internal/vm/bin/sync_socket_patch.dart
M sdk/lib/io/socket.dart
M sdk/lib/io/sync_socket.dart
A tests/standalone/io/socket_address_test.dart
A tests/standalone_2/io/socket_address_test.dart
23 files changed, 383 insertions(+), 87 deletions(-)
diff --git a/BREAKING-CHANGE-REQUEST b/BREAKING-CHANGE-REQUEST
new file mode 100644
index 0000000..8df9543
--- /dev/null
+++ b/BREAKING-CHANGE-REQUEST
@@ -0,0 +1,45 @@
+Breaking Change Request: Fix .address not being the local address for client sockets
+
+Label: breaking-change-request
+
+# Proposal
+
+We propose fixing the `address` getter on the `Socket`, `RawSocket`, and
+`RawSynchronousSocket` classes so it always returns the local address.
+Previously it would return the remote address for client side connections,
+despite the remote address being also available in `remoteAddress`. Server side
+connections already return the local address in the `address` getter.
+
+A proposed implementation is at
+<https://dart-review.googlesource.com/c/sdk/+/127465>.
+
+# Rationale
+
+There is currently no direct way for client side connections to find the local
+address that was bound to and the behavior was inconsistent with server side
+connections.
+
+The socket implementation is generally confused about local/remote addreses and
+ports which results in bugs like #12693. The proposed implementation has a much
+cleaner design that fits well with the underlying `getpeername` and
+`getsockname` system calls and user expectations.
+
+# Impact
+
+All existing programs continue to compile and the difference is observeable at
+runtime. The impact is expected to be low.
+
+The `address` getter is mostly used for logging which will won't break, but the
+logged address may now unexpectedly be the local address instead of the remote
+address.
+
+No cases have been found in a code search where the code actually wanted the
+remote address, and cases were found that would want the local address.
+
+# Mitigation
+
+Programs that want the remote address can use the `remoteAddress` getter instead
+which has always worked and is backwards compatible.
+
+Programs that wish to depend on the `address` getter providing the local address
+for client connections can increase their minimum SDK version.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2ae1e14..036a2ca 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,22 @@
### Core libraries
+#### `dart:io`
+
+* **Breaking change** [#TBA][]: The `address` getter on the `Socket`,
+ `RawSocket`, and `RawSynchronousSocket` classes now provides the socket's
+ local address for client connections. Previously for client connections it
+ would report the socket's remote address, which is also available in the
+ `remoteAddress` getter. Sockets accepted on the server side always provided
+ the correct local address in the `address` getter and the correct remote
+ address in the `remoteAddress` getter.
+
+ Code that relies on `address` actually being `remoteAddress` on the client
+ side needs to be updated to use `remoteAddress` instead, which is backwards
+ compatible.
+
+[#TBA]: https://github.com/dart-lang/sdk/issues/TBA
+
### Dart VM
### Tools
diff --git a/runtime/bin/io_natives.cc b/runtime/bin/io_natives.cc
index 9730817..a2c67eb 100644
--- a/runtime/bin/io_natives.cc
+++ b/runtime/bin/io_natives.cc
@@ -143,7 +143,7 @@
V(Socket_CreateBindDatagram, 6) \
V(Socket_CreateConnect, 4) \
V(Socket_CreateUnixDomainConnect, 3) \
- V(Socket_GetPort, 1) \
+ V(Socket_GetSocketName, 1) \
V(Socket_GetRemotePeer, 1) \
V(Socket_GetError, 1) \
V(Socket_GetOption, 3) \
@@ -172,7 +172,7 @@
V(SynchronousSocket_Available, 1) \
V(SynchronousSocket_CloseSync, 1) \
V(SynchronousSocket_CreateConnectSync, 3) \
- V(SynchronousSocket_GetPort, 1) \
+ V(SynchronousSocket_GetSocketName, 1) \
V(SynchronousSocket_GetRemotePeer, 1) \
V(SynchronousSocket_LookupRequest, 2) \
V(SynchronousSocket_ShutdownRead, 1) \
diff --git a/runtime/bin/socket.cc b/runtime/bin/socket.cc
index 6769a93..3366622 100644
--- a/runtime/bin/socket.cc
+++ b/runtime/bin/socket.cc
@@ -741,14 +741,32 @@
}
}
-void FUNCTION_NAME(Socket_GetPort)(Dart_NativeArguments args) {
+void FUNCTION_NAME(Socket_GetSocketName)(Dart_NativeArguments args) {
Socket* socket =
Socket::GetSocketIdNativeField(Dart_GetNativeArgument(args, 0));
- intptr_t port = SocketBase::GetPort(socket->fd());
- if (port > 0) {
- Dart_SetIntegerReturnValue(args, port);
+ OSError os_error;
+ intptr_t port = 0;
+ SocketAddress* addr = SocketBase::GetSocketName(socket->fd(), &port);
+ if (addr != nullptr) {
+ Dart_Handle list = Dart_NewList(2);
+ int type = addr->GetType();
+ Dart_Handle entry;
+ if (type == SocketAddress::TYPE_UNIX) {
+ entry = Dart_NewList(2);
+ } else {
+ entry = Dart_NewList(3);
+ RawAddr raw = addr->addr();
+ Dart_ListSetAt(entry, 2, SocketAddress::ToTypedData(raw));
+ }
+ Dart_ListSetAt(entry, 0, Dart_NewInteger(type));
+ Dart_ListSetAt(entry, 1, Dart_NewStringFromCString(addr->as_string()));
+
+ Dart_ListSetAt(list, 0, entry);
+ Dart_ListSetAt(list, 1, Dart_NewInteger(port));
+ Dart_SetReturnValue(args, list);
+ delete addr;
} else {
- Dart_SetReturnValue(args, DartUtils::NewDartOSError());
+ Dart_ThrowException(DartUtils::NewDartOSError());
}
}
diff --git a/runtime/bin/socket_base.cc b/runtime/bin/socket_base.cc
index e90b138..29b6140 100644
--- a/runtime/bin/socket_base.cc
+++ b/runtime/bin/socket_base.cc
@@ -222,6 +222,16 @@
}
}
+intptr_t SocketBase::GetPort(intptr_t fd) {
+ intptr_t port;
+ SocketAddress* address = SocketBase::GetSocketName(fd, &port);
+ if (address == nullptr) {
+ return 0;
+ }
+ delete address;
+ return port;
+}
+
void FUNCTION_NAME(InternetAddress_Parse)(Dart_NativeArguments args) {
const char* address =
DartUtils::GetStringValue(Dart_GetNativeArgument(args, 0));
diff --git a/runtime/bin/socket_base.h b/runtime/bin/socket_base.h
index bf9405e..181d948 100644
--- a/runtime/bin/socket_base.h
+++ b/runtime/bin/socket_base.h
@@ -190,6 +190,7 @@
// to bind the socket to a specific IP.
static bool IsBindError(intptr_t error_number);
static intptr_t GetPort(intptr_t fd);
+ static SocketAddress* GetSocketName(intptr_t fd, intptr_t* port);
static SocketAddress* GetRemotePeer(intptr_t fd, intptr_t* port);
static void GetError(intptr_t fd, OSError* os_error);
static int GetType(intptr_t fd);
diff --git a/runtime/bin/socket_base_fuchsia.cc b/runtime/bin/socket_base_fuchsia.cc
index be1eb25..cad9229 100644
--- a/runtime/bin/socket_base_fuchsia.cc
+++ b/runtime/bin/socket_base_fuchsia.cc
@@ -163,16 +163,18 @@
return -1;
}
-intptr_t SocketBase::GetPort(intptr_t fd) {
+SocketAddress* SocketBase::GetSocketName(intptr_t fd, intptr_t* port) {
IOHandle* handle = reinterpret_cast<IOHandle*>(fd);
ASSERT(handle->fd() >= 0);
RawAddr raw;
socklen_t size = sizeof(raw);
- LOG_INFO("SocketBase::GetPort: calling getsockname(%ld)\n", handle->fd());
+ LOG_INFO("SocketBase::GetSocketName: calling getsockname(%ld)\n",
+ handle->fd());
if (NO_RETRY_EXPECTED(getsockname(handle->fd(), &raw.addr, &size))) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/socket_base_linux.cc b/runtime/bin/socket_base_linux.cc
index 8734aa6..ff1d5b3 100644
--- a/runtime/bin/socket_base_linux.cc
+++ b/runtime/bin/socket_base_linux.cc
@@ -140,14 +140,15 @@
return written_bytes;
}
-intptr_t SocketBase::GetPort(intptr_t fd) {
+SocketAddress* SocketBase::GetSocketName(intptr_t fd, intptr_t* port) {
ASSERT(fd >= 0);
RawAddr raw;
socklen_t size = sizeof(raw);
if (NO_RETRY_EXPECTED(getsockname(fd, &raw.addr, &size))) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/socket_base_macos.cc b/runtime/bin/socket_base_macos.cc
index 74bf924..1f176e7 100644
--- a/runtime/bin/socket_base_macos.cc
+++ b/runtime/bin/socket_base_macos.cc
@@ -139,14 +139,15 @@
return written_bytes;
}
-intptr_t SocketBase::GetPort(intptr_t fd) {
+SocketAddress* SocketBase::GetSocketName(intptr_t fd, intptr_t* port) {
ASSERT(fd >= 0);
RawAddr raw;
socklen_t size = sizeof(raw);
if (NO_RETRY_EXPECTED(getsockname(fd, &raw.addr, &size))) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/socket_base_win.cc b/runtime/bin/socket_base_win.cc
index d762ad9..ac49e33 100644
--- a/runtime/bin/socket_base_win.cc
+++ b/runtime/bin/socket_base_win.cc
@@ -125,15 +125,19 @@
SocketAddress::GetAddrLength(addr));
}
-intptr_t SocketBase::GetPort(intptr_t fd) {
+SocketAddress* SocketBase::GetSocketName(intptr_t fd, intptr_t* port) {
ASSERT(reinterpret_cast<Handle*>(fd)->is_socket());
SocketHandle* socket_handle = reinterpret_cast<SocketHandle*>(fd);
RawAddr raw;
socklen_t size = sizeof(raw);
if (getsockname(socket_handle->socket(), &raw.addr, &size) == SOCKET_ERROR) {
- return 0;
+ return NULL;
}
- return SocketAddress::GetAddrPort(raw);
+ *port = SocketAddress::GetAddrPort(raw);
+ // Clear the port before calling WSAAddressToString as WSAAddressToString
+ // includes the port in the formatted string.
+ SocketAddress::SetAddrPort(&raw, 0);
+ return new SocketAddress(&raw.addr);
}
SocketAddress* SocketBase::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket.cc b/runtime/bin/sync_socket.cc
index c8e7714..45ce809 100644index 80094f7..db4293a 100644
--- a/runtime/bin/sync_socket_android.cc
+++ b/runtime/bin/sync_socket_android.cc
@@ -54,8 +54,8 @@
return SocketBase::Available(fd);
}
-intptr_t SynchronousSocket::GetPort(intptr_t fd) {
- return SocketBase::GetPort(fd);
+SocketAddress* SynchronousSocket::GetSocketName(intptr_t fd, intptr_t* port) {
+ return SocketBase::GetSocketName(fd, port);
}
SocketAddress* SynchronousSocket::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket_fuchsia.cc b/runtime/bin/sync_socket_fuchsia.cc
index 23461f4f..823d170 100644
--- a/runtime/bin/sync_socket_fuchsia.cc
+++ b/runtime/bin/sync_socket_fuchsia.cc
@@ -54,8 +54,8 @@
return SocketBase::Available(fd);
}
-intptr_t SynchronousSocket::GetPort(intptr_t fd) {
- return SocketBase::GetPort(fd);
+SocketAddress* SynchronousSocket::GetSocketName(intptr_t fd, intptr_t* port) {
+ return SocketBase::GetSocketName(fd, port);
}
SocketAddress* SynchronousSocket::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket_linux.cc b/runtime/bin/sync_socket_linux.cc
index 963787b..18dcdf1 100644
--- a/runtime/bin/sync_socket_linux.cc
+++ b/runtime/bin/sync_socket_linux.cc
@@ -54,8 +54,8 @@
return SocketBase::Available(fd);
}
-intptr_t SynchronousSocket::GetPort(intptr_t fd) {
- return SocketBase::GetPort(fd);
+SocketAddress* SynchronousSocket::GetSocketName(intptr_t fd, intptr_t* port) {
+ return SocketBase::GetSocketName(fd, port);
}
SocketAddress* SynchronousSocket::GetRemotePeer(intptr_t fd, intptr_t* port) {
diff --git a/runtime/bin/sync_socket_macos.cc b/runtime/bin/sync_socket_macos.cc
index e266341..31a045f 100644index 339d9a7..2be965c 100644
--- a/sdk/lib/_internal/vm/bin/socket_patch.dart
+++ b/sdk/lib/_internal/vm/bin/socket_patch.dart
@@ -460,11 +460,10 @@
// The type flags for this socket.
final int typeFlags;
- // Holds the port of the socket, 0 if not known.
- int localPort = 0;
-
- // Holds the address used to connect or bind the socket.
- late InternetAddress localAddress;
+ // The requested remote address and port, for error messages in case the
+ // connection fails.
+ final InternetAddress? requestedAddress;
+ final int? requestedPort;
// The size of data that is ready to be read, for TCP sockets.
// This might be out-of-date when Read is called.
@@ -785,7 +784,7 @@
return;
}
final address = pendingLookedUp.removeFirst();
- final socket = new _NativeSocket.normal(address);
+ final socket = new _NativeSocket.normal(address, port);
// Will contain values of various types representing the result
// of trying to create a connection.
// A value of `true` means success, everything else means failure.
@@ -928,7 +927,7 @@
}
final address = await _resolveHost(host);
- var socket = new _NativeSocket.listen(address);
+ var socket = new _NativeSocket.listen(address, port);
var result;
if (address.type == InternetAddressType.unix) {
var path = address.address;
@@ -945,7 +944,6 @@
throw new SocketException("Failed to create server socket",
osError: result, address: address, port: port);
}
- if (port != 0) socket.localPort = port;
socket.connectToEventHandler();
return socket;
}
@@ -957,32 +955,42 @@
final address = await _resolveHost(host);
- var socket = new _NativeSocket.datagram(address);
+ var socket = new _NativeSocket.datagram(address, port);
var result = socket.nativeCreateBindDatagram(
address._in_addr, port, reuseAddress, reusePort, ttl);
if (result is OSError) {
throw new SocketException("Failed to create datagram socket",
osError: result, address: address, port: port);
}
- if (port != 0) socket.localPort = port;
return socket;
}
- _NativeSocket.datagram(this.localAddress)
- : typeFlags = typeNormalSocket | typeUdpSocket;
+ _NativeSocket.datagram(InternetAddress requestedAddress, int requestedPort)
+ : typeFlags = typeNormalSocket | typeUdpSocket,
+ requestedAddress = requestedAddress,
+ requestedPort = requestedPort;
- _NativeSocket.normal(this.localAddress)
- : typeFlags = typeNormalSocket | typeTcpSocket;
+ _NativeSocket.normal(InternetAddress requestedAddress, int requestedPort)
+ : typeFlags = typeNormalSocket | typeTcpSocket,
+ requestedAddress = requestedAddress,
+ requestedPort = requestedPort;
- _NativeSocket.listen(this.localAddress)
- : typeFlags = typeListeningSocket | typeTcpSocket {
+ _NativeSocket.listen(InternetAddress requestedAddress, int requestedPort)
+ : typeFlags = typeListeningSocket | typeTcpSocket,
+ requestedAddress = requestedAddress,
+ requestedPort = requestedPort {
isClosedWrite = true;
}
- _NativeSocket.pipe() : typeFlags = typePipe;
+ _NativeSocket.pipe()
+ : typeFlags = typePipe,
+ requestedAddress = null,
+ requestedPort = null;
_NativeSocket._watchCommon(int id, int type)
- : typeFlags = typeNormalSocket | type {
+ : typeFlags = typeNormalSocket | type,
+ requestedAddress = null,
+ requestedPort = null {
isClosedWrite = true;
nativeSetSocketId(id, typeFlags);
}
@@ -1135,26 +1143,32 @@
connections--;
tokens++;
returnTokens(listeningTokenBatchSize);
- var socket = new _NativeSocket.normal(address);
+ var socket = new _NativeSocket.normal(address, port);
if (nativeAccept(socket) != true) return null;
- socket.localPort = localPort;
return socket;
}
int get port {
- if (localAddress.type == InternetAddressType.unix) return 0;
- if (localPort != 0) return localPort;
if (isClosing || isClosed) throw const SocketException.closed();
- return localPort = nativeGetPort();
+ return nativeGetSocketName()[1];
}
int get remotePort {
- if (localAddress.type == InternetAddressType.unix) return 0;
if (isClosing || isClosed) throw const SocketException.closed();
return nativeGetRemotePeer()[1];
}
- InternetAddress get address => localAddress;
+ InternetAddress get address {
+ if (isClosing || isClosed) throw const SocketException.closed();
+ var result = nativeGetSocketName();
+ var addr = result[0];
+ var type = new InternetAddressType._from(addr[0]);
+ if (type == InternetAddressType.unix) {
+ return _InternetAddress.fromString(addr[1],
+ type: InternetAddressType.unix);
+ }
+ return _InternetAddress(type, addr[1], null, addr[2]);
+ }
InternetAddress get remoteAddress {
if (isClosing || isClosed) throw const SocketException.closed();
@@ -1424,8 +1438,21 @@
}
void reportError(error, StackTrace? st, String message) {
- var e =
- createError(error, message, isUdp || isTcp ? address : null, localPort);
+ InternetAddress? errorAddress;
+ int? errorPort;
+ if (isTcp || isUdp) {
+ try {
+ // These will throw an OSError ENOTCONN if the connection fails.
+ errorAddress = remoteAddress;
+ errorPort = port;
+ } on OSError catch (_) {
+ // Fall back on the requested remote information if we can't get the
+ // authoritative information.
+ errorAddress = requestedAddress;
+ errorPort = requestedPort;
+ }
+ }
+ var e = createError(error, message, requestedAddress, requestedPort);
// Invoke the error handler if any.
if (eventHandlers[errorEvent] != null) {
eventHandlers[errorEvent](e, st);
@@ -1529,7 +1556,7 @@
nativeCreateBindDatagram(Uint8List addr, int port, bool reuseAddress,
bool reusePort, int ttl) native "Socket_CreateBindDatagram";
bool nativeAccept(_NativeSocket socket) native "ServerSocket_Accept";
- int nativeGetPort() native "Socket_GetPort";
+ List nativeGetSocketName() native "Socket_GetSocketName";
List nativeGetRemotePeer() native "Socket_GetRemotePeer";
int nativeGetSocketId() native "Socket_GetSocketId";
OSError nativeGetError() native "Socket_GetError";
diff --git a/sdk/lib/_internal/vm/bin/sync_socket_patch.dart b/sdk/lib/_internal/vm/bin/sync_socket_patch.dart
index 6554c11..df7fa30 100644
--- a/sdk/lib/_internal/vm/bin/sync_socket_patch.dart
+++ b/sdk/lib/_internal/vm/bin/sync_socket_patch.dart
@@ -54,12 +54,6 @@
bool isClosedRead = false;
bool isClosedWrite = false;
- // Holds the address used to connect the socket.
- late InternetAddress localAddress;
-
- // Holds the port of the socket, 0 if not known.
- int localPort = 0;
-
static _NativeSynchronousSocket connectSync(host, int port) {
if (host == null) {
throw new ArgumentError("Parameter host cannot be null");
@@ -87,7 +81,6 @@
}
var address = it.current;
var socket = new _NativeSynchronousSocket();
- socket.localAddress = address;
var result = socket._nativeCreateConnectSync(address._in_addr, port);
if (result is OSError) {
// Keep first error, if present.
@@ -112,21 +105,34 @@
return connectNext();
}
- InternetAddress get address => localAddress;
int get available => _nativeAvailable();
- int get port {
- if (localPort != 0) {
- return localPort;
- }
+ InternetAddress get address {
if (isClosed) {
throw const SocketException.closed();
}
- var result = _nativeGetPort();
+ var result = _nativeGetSocketName();
if (result is OSError) {
throw result;
}
- return localPort = result;
+ var addr = result[0];
+ var type = InternetAddressType._from(addr[0]);
+ if (type == InternetAddressType.unix) {
+ return _InternetAddress.fromString(addr[1],
+ type: InternetAddressType.unix);
+ }
+ return _InternetAddress(type, addr[1], null, addr[2]);
+ }
+
+ int get port {
+ if (isClosed) {
+ throw const SocketException.closed();
+ }
+ var result = _nativeGetSocketName();
+ if (result is OSError) {
+ throw result;
+ }
+ return result[1];
}
InternetAddress get remoteAddress {
@@ -304,8 +310,8 @@
native "SynchronousSocket_CreateConnectSync";
_nativeAvailable() native "SynchronousSocket_Available";
_nativeCloseSync() native "SynchronousSocket_CloseSync";
- int _nativeGetPort() native "SynchronousSocket_GetPort";
- List _nativeGetRemotePeer() native "SynchronousSocket_GetRemotePeer";
+ _nativeGetSocketName() native "SynchronousSocket_GetSocketName";
+ _nativeGetRemotePeer() native "SynchronousSocket_GetRemotePeer";
_nativeRead(int len) native "SynchronousSocket_Read";
_nativeReadInto(List<int> buffer, int offset, int bytes)
native "SynchronousSocket_ReadList";
diff --git a/sdk/lib/io/socket.dart b/sdk/lib/io/socket.dart
index 22d57dd..ea12f44 100644
--- a/sdk/lib/io/socket.dart
+++ b/sdk/lib/io/socket.dart
@@ -626,7 +626,7 @@
/// `buffer.length - offset`.
int write(List<int> buffer, [int offset = 0, int? count]);
- /// The port used by this socket.
+ /// The local port used by this socket.
///
/// Throws a [SocketException] if the socket is closed.
int get port;
@@ -636,9 +636,13 @@
/// Throws a [SocketException] if the socket is closed.
int get remotePort;
- /// The [InternetAddress] used to connect this socket.
+ /// The local [InternetAddress] used by this socket.
///
/// Throws a [SocketException] if the socket is closed.
+ ///
+ /// Note: Prior to Dart 2.13, client side sockets would return the remote
+ /// address instead of the local address. Sockets accepted on the server side
+ /// always returned the local address.
InternetAddress get address;
/// The remote [InternetAddress] connected to by this socket.
@@ -784,7 +788,7 @@
/// been destroyed or upgraded to a secure socket.
void setRawOption(RawSocketOption option);
- /// The port used by this socket.
+ /// The local port used by this socket.
///
/// Throws a [SocketException] if the socket is closed.
/// The port is 0 if the socket is a Unix domain socket.
@@ -796,9 +800,13 @@
/// The port is 0 if the socket is a Unix domain socket.
int get remotePort;
- /// The [InternetAddress] used to connect this socket.
+ /// The local [InternetAddress] used by this socket.
///
/// Throws a [SocketException] if the socket is closed.
+ ///
+ /// Note: Prior to Dart 2.13, client side sockets would return the remote
+ /// address instead of the local address. Sockets accepted on the server side
+ /// always returned the local address.
InternetAddress get address;
/// The remote [InternetAddress] connected to by this socket.
diff --git a/sdk/lib/io/sync_socket.dart b/sdk/lib/io/sync_socket.dart
index e1e47e5..dc10990 100644
--- a/sdk/lib/io/sync_socket.dart
+++ b/sdk/lib/io/sync_socket.dart
@@ -72,13 +72,17 @@
/// and no greater than `buffer.length`.
void writeFromSync(List<int> buffer, [int start = 0, int? end]);
- /// The port used by this socket.
+ /// The local port used by this socket.
int get port;
/// The remote port connected to by this socket.
int get remotePort;
- /// The [InternetAddress] used to connect this socket.
+ /// The local [InternetAddress] used by this socket.
+ ///
+ /// Note: Prior to Dart 2.13, client side sockets would return the remote
+ /// address instead of the local address. Sockets accepted on the server side
+ /// always returned the local address.
InternetAddress get address;
/// The remote [InternetAddress] connected to by this socket.
diff --git a/tests/standalone/io/socket_address_test.dart b/tests/standalone/io/socket_address_test.dart
new file mode 100644
index 0000000..5519b3f
--- /dev/null
+++ b/tests/standalone/io/socket_address_test.dartJonas Termansen (Gerrit)
Mar 11, 2021, 11:23:26 AM3/11/21
to rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Alexander Aprelev.
Patch set 3:Commit-Queue +1
4 comments:
Patchset:
Dusting off this old pet dragon bug of mine for the hackathon. It's technically a breaking change so prepared to go through that process. I agreed on the design with Lasse a couple years back.
I have a little bit of uncertainty about ABI stability for native calls on the VM side.
File BREAKING-CHANGE-REQUEST:
Patch Set #3, Line 37:
remote address, and cases were found that would want the local address.We did this search two years ago. I didn't find much. It's difficult to do a proper search though.
File runtime/bin/io_natives.cc:
Patch Set #3, Line 146:
V(Socket_GetSocketName, 1) \Changing this native call means an older sdk lib can't be used with this VM. Is that a problem? Not sure what kinds of ABI stability the VM currently has. If so, we can retain the Socket_GetPort native and bump some minor compatible version number?
File sdk/lib/io/socket.dart:
Patch Set #3, Line 643:
Dart 2.13I assume that's next?
Do we want to have such historical notes in here so people can set their minimum SDK versions? I think it's a bit handy for users, at least for a while, if it's maintainable. We don't want to add @Since because this retains a useful behavior for server side connections.
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Jonas Termansen (Gerrit)
Mar 11, 2021, 11:23:27 AM3/11/21
to rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Alexander Aprelev.
Jonas Termansen removed Zichang Guo from this change.
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Jonas Termansen (Gerrit)
Mar 11, 2021, 11:57:09 AM3/11/21
to rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Alexander Aprelev.
Patch set 3:Commit-Queue +1
3 comments:
File sdk/lib/_internal/vm/bin/socket_patch.dart:
Patch Set #3, Line 1152:
if (isClosing || isClosed) throw const SocketException.closed();Hmm. This removes the caching behavior where it didn't throw, which is potentially also a breaking change.
Patch Set #3, Line 1162:
if (isClosing || isClosed) throw const SocketException.closed();Hmm. This additional throw is also a breaking change.
Patch Set #3, Line 1170:
return _InternetAddress(type, addr[1], null, addr[2]);This is causing test failures because it doesn't contain the host name used to look up the address. This matters for the HttpServer in a few cases, such as when the Host field isn't set, when requestedUri uses _httpServer.address.host to find the hostname bound to.
I think we can solve that by keeping around the hostname, if any, used for the local side and supplying it here. It's not semantically right to retain the requested local address because connecting from e.g. 0.0.0.0 (default) you would get an actual local address assigned when the connection happens.
Supplying the hostname here is inconsistent though with the remoteAddress call below that does not. Perhaps we should also do this for the remote address. That's also observable but not really breaking as such.
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Jonas Termansen (Gerrit)
Mar 15, 2021, 9:56:09 AM3/15/21
to rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Alexander Aprelev.
Patch set 4:Commit-Queue +1
2 comments:
Patchset:
OK, this should fix more of the tests. There should still be a couple minor failures left where the tests thinks they can look at the local address/port after the socket has been closed, those tests needs to be updated.
There's a couple new behaviors in this patchset, like retaining the host for remoteAddress too, and also fixing address/remoteAddress confusion in the secure socket code. Those may require a couple commit message / documentation / changelog / breaking change mentions, I'll consider those observable semantic differences.
File sdk/lib/_internal/vm/bin/socket_patch.dart:
Patch Set #3, Line 1170:
return _InternetAddress(type, addr[1], null, addr[2]);This is causing test failures because it doesn't contain the host name used to look up the address. […]
Done
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Dart CI (Gerrit)
Mar 15, 2021, 10:09:24 AM3/15/21
to Jonas Termansen, rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Alexander Aprelev.
go/dart-cbuild result: FAILURE (REGRESSIONS DETECTED)
Details: https://goto.google.com/dart-cbuild/find/729f230fbd396dc891f94cd07928c8b8f8f79fa4
Bugs: go/dart-cbuild-bug/729f230fbd396dc891f94cd07928c8b8f8f79fa4
Dart CI (Gerrit)
Mar 15, 2021, 10:25:13 AM3/15/21
to Jonas Termansen, rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Lasse R.H. Nielsen, Alexander Aprelev.
go/dart-cbuild result: FAILURE (REGRESSIONS DETECTED)
Details: https://goto.google.com/dart-cbuild/find/9230465b2f4743ea577da5977324dcebbced4049
Bugs: go/dart-cbuild-bug/9230465b2f4743ea577da5977324dcebbced4049
Jonas Termansen (Gerrit)
Mar 16, 2021, 11:23:14 AM3/16/21
to rev...@dartlang.org, vm-...@dartlang.org, Dart CI, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Lasse R.H. Nielsen, Alexander Aprelev.
Patch set 5:Commit-Queue +1
1 comment:
File sdk/lib/_internal/vm/bin/socket_patch.dart:
Patch Set #5, Line 1185:
localHostHmm. Could this be "0.0.0.0" and the local address is something different?
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Jonas Termansen (Gerrit)
Mar 16, 2021, 11:29:27 AM3/16/21
to rev...@dartlang.org, vm-...@dartlang.org, Dart CI, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Lasse R.H. Nielsen, Alexander Aprelev.
1 comment:
File BREAKING-CHANGE-REQUEST:
+The `.port` and `.address` getters on all the socket classes will now throw a `SocketException `if the socket is closing or has been closed. This behavior matches `remoteAddress` and `remotePort`.
(This behavior may also propagate to classes like HttpServer)
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Dart CI (Gerrit)
Mar 16, 2021, 12:07:52 PM3/16/21
to Jonas Termansen, rev...@dartlang.org, vm-...@dartlang.org, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Lasse R.H. Nielsen, Alexander Aprelev.
go/dart-cbuild result: SUCCESS
Details: https://goto.google.com/dart-cbuild/find/5b4434d001c0c570650a7af65be6eb04dce6334a
Jonas Termansen (Gerrit)
Mar 17, 2021, 8:27:08 AM3/17/21
to rev...@dartlang.org, vm-...@dartlang.org, Dart CI, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Attention is currently required from: Lasse R.H. Nielsen, Alexander Aprelev.
1 comment:
File runtime/bin/socket_base_win.cc:
Patch Set #5, Line 133:
if (getsockname(socket_handle->socket(), &raw.addr, &size)) {This fails for a strange reason since this code is identical to the case below. I wonder if the socket is in the wrong state or if it's something else.
OS Error: The system detected an invalid pointer address in attempting to use a pointer argument in a call.
, errno = 10014
#0 _NativeSocket.nativeGetSocketName (dart:io-patch/socket_patch.dart:1574:59)
#1 _NativeSocket.address (dart:io-patch/socket_patch.dart:1178:18)
#2 _RawServerSocket.listen.<anonymous closure> (dart:io-patch/socket_patch.dart:1625:38)
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Jonas Termansen (Gerrit)
Mar 24, 2021, 7:28:27 AM3/24/21
to rev...@dartlang.org, vm-...@dartlang.org, Dart CI, Alexander Aprelev, commi...@chromium.org, Lasse R.H. Nielsen
Patchset:
@lrn, any thoughts?
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Lasse R.H. Nielsen (Gerrit)
Mar 24, 2021, 8:14:57 AM3/24/21
to Jonas Termansen, rev...@dartlang.org, vm-...@dartlang.org, Dart CI, Alexander Aprelev, commi...@chromium.org
Attention is currently required from: Jonas Termansen, Alexander Aprelev.
Patch set 6:Code-Review +1
5 comments:
Patchset:
Looks good to me, but I don't actually understand sockets.
File sdk/lib/_internal/vm/bin/socket_patch.dart:
Patch Set #3, Line 1152:
if (isClosing || isClosed) throw const SocketException.closed();Hmm. […]
I'll allow it. 😊
It was bad behavior to throw if reading after a close, but not do so if you had read just once before closing. That behavior encourages you to do spurious reads just to cache the value in case you need it after close.
Either always work or never work after a close. So, this seems fine.
File sdk/lib/_internal/vm/bin/socket_patch.dart:
Patch Set #6, Line 464:
// connection fails.Even if this isn't a doc-comment, I'd still split it into a short first line and elaborative following paragraphs.
// The requested remote address and port, if any.
//
// Used by error messages if the connection fails.I added "if any" because it's nullable.
Patch Set #6, Line 468:
// The requested local and remote hostnames, which are filled into whateverwhatever ... are -> the ... which are
Patch Set #6, Line 1468:
} on OSError catch (_) {} on SocketException catch (_) {}You can drop the `catch(_)`, an `on OSError {}` is valid by itself.
This is formatted oddly. Is the formatter to blame? If so, maybe file an issue against it. It shouldn't put two `on` clauses on the same line.
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Jonas Termansen (Gerrit)
Mar 24, 2021, 12:02:25 PM3/24/21
to rev...@dartlang.org, vm-...@dartlang.org, Lasse R.H. Nielsen, Dart CI, Alexander Aprelev, commi...@chromium.org
Attention is currently required from: Alexander Aprelev.
1 comment:
Patchset:
@aam: Any thoughts on the VM side of things or anything else in general?
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Aprelev (Gerrit)
Apr 1, 2021, 12:19:51 PM4/1/21
to Jonas Termansen, rev...@dartlang.org, vm-...@dartlang.org, Lasse R.H. Nielsen, Dart CI, Alexander Aprelev, commi...@chromium.org
Attention is currently required from: Jonas Termansen.
11 comments:
Commit Message:
This change fixes the .address getter so it always returns the local
address
Could we ease the pain of breaking change by introducing |localAddress|, |localPort| and marking |address| deprecated potentially providing automatic dartfix for the migration?
File runtime/bin/io_natives.cc:
Patch Set #3, Line 146:
V(Socket_GetSocketName, 1) \
Changing this native call means an older sdk lib can't be used with this VM. […]
There are no explicit API stability guarantees provided by Dart VM as far as I know.
File runtime/bin/socket_base_android.cc:
Patch Set #6, Line 144:
ASSERT(fd >= 0);ASSERT(port != nullptr); ?
We try to use nullptr in new/refactored code
Patch Set #6, Line 150:
sun_familtysun_family
Patch Set #6, Line 151:
sa_familty_tsa_family_t
Patch Set #6, Line 154:
*port = 0;Previously GetPort would always get port number, but now it seems that it would return 0 in case of "unnamed socket". Is this intentional?
/*unnamed_unix_socket=*/true
File runtime/bin/socket_base_win.cc:
Patch Set #6, Line 133:
== SOCKET_ERRORI like how we explicitly checked for SOCKET_ERROR, rather than 0.
Was there a reason to change it?
File sdk/lib/_internal/vm/bin/socket_patch.dart:
/*localHost=*/null
or make that parameter named optional?
These will throw an OSError ENOTCONN if the connection fails, or
// SocketException if the connection is closing or closed.Is this applicable to Windows too?
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Aprelev (Gerrit)
Sep 10, 2021, 4:00:22 PM9/10/21
to Jonas Termansen, rev...@dartlang.org, vm-...@dartlang.org, Lasse R.H. Nielsen, Dart CI, Alexander Aprelev, commi...@chromium.org
Attention is currently required from: Jonas Termansen.
1 comment:
Patchset:
I think this is still good change to land. This inconsistency in `address` property value between server socket and client socket is problematic. Do you plan on getting back to this Jonas by any chance?
To view, visit change 127465. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages