Added synchronous socket implementation to dart:io.

runtime/bin/sync_socket.cc

Index: runtime/bin/sync_socket.cc
diff --git a/runtime/bin/sync_socket.cc b/runtime/bin/sync_socket.cc
new file mode 100644
index 0000000000000000000000000000000000000000..42d041edc805ffd1ca289943db8f7e7bd0aea846
--- /dev/null
+++ b/runtime/bin/sync_socket.cc
@@ -0,0 +1,328 @@
+// Copyright (c) 2017, 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.
+
+#if !defined(DART_IO_DISABLED)
+
+#include "bin/sync_socket.h"
+
+#include "bin/dartutils.h"
+#include "bin/io_buffer.h"
+#include "bin/isolate_data.h"
+#include "bin/lockers.h"
+#include "bin/thread.h"
+#include "bin/utils.h"
+
+#include "include/dart_api.h"
+
+#include "platform/globals.h"
+#include "platform/utils.h"
+
+namespace dart {
+namespace bin {
+
+static const int kSocketIdNativeField = 0;

[siva, 2017/04/10 22:51:46]

I have one high level comment, maybe does not need to be addressed in this CL
but possibly in a follow-up CL.

The code in this file seems very similar to the code in socket.cc, maybe it is
possible to share between these two files.

[bkonyi, 2017/04/11 01:30:50]

They are very similar, but I think I've gotten most of the code that can be
shared pulled into SocketBase. I'll take another look to see if we can pull more
out into SocketBase in another CL.

+
+
+void FUNCTION_NAME(SynchronousSocket_LookupRequest)(Dart_NativeArguments args) {
+  if (Dart_GetNativeArgumentCount(args) != 2) {
+    Dart_SetReturnValue(
+        args, DartUtils::NewDartArgumentError("Invalid argument count."));
+    return;
+  }
+
+  char* host = NULL;
+  char* peer = NULL;
+  int64_t type = 0;
+
+  Dart_Handle host_arg =
+      Dart_GetNativeStringArgument(args, 0, reinterpret_cast<void**>(&peer));
+  if (Dart_IsError(host_arg)) {
+    Dart_SetReturnValue(
+        args, DartUtils::NewDartArgumentError("Invalid value for host"));

[zra, 2017/04/10 21:39:43]

host_arg is already the error that we want to propagate here, so there's no need
to make a new one.

[bkonyi, 2017/04/11 01:30:50]

Done.

+    return;
+  }
+
+  host_arg = Dart_StringToCString(host_arg, const_cast<const char**>(&host));
+  if (Dart_IsError(host_arg)) {
+    Dart_SetReturnValue(
+        args, DartUtils::NewDartArgumentError("Invalid value for host"));

[zra, 2017/04/10 21:39:42]

ditto

[bkonyi, 2017/04/11 01:30:50]

Done.

+    return;
+  }
+
+  Dart_Handle port_error = Dart_GetNativeIntegerArgument(args, 1, &type);
+  if (Dart_IsError(port_error)) {
+    Dart_SetReturnValue(
+        args, DartUtils::NewDartArgumentError("Invalid value for port"));

[zra, 2017/04/10 21:39:43]

ditto

[bkonyi, 2017/04/11 01:30:50]

Done.

+    return;
+  }
+
+  OSError* os_error = NULL;
+  Dart_Handle result = NULL;
+  AddressList<SocketAddress>* addresses =
+      SocketBase::LookupAddress(host, type, &os_error);
+
+  if (addresses == NULL) {
+    Dart_SetReturnValue(args, DartUtils::NewDartOSError(os_error));
+    return;
+  }
+
+  Dart_Handle array = Dart_NewList(addresses->count());
+  DART_CHECK_VALID_AND_PROPAGATE(array);

[zra, 2017/04/10 21:39:43]

This leaks 'addresses'.

Here and below, we should instead be doing:

if (Dart_IsError(handle)) {
  delete addresses;
  Dart_SetReturnValue(args, handle);
  return;
}

You can wrap that up locally in this file with a macro.

[bkonyi, 2017/04/11 01:30:51]

Done.

+
+  for (intptr_t i = 0; i < addresses->count(); i++) {
+    SocketAddress* addr = addresses->GetAt(i);
+    Dart_Handle entry = Dart_NewList(3);
+    DART_CHECK_VALID_AND_PROPAGATE(entry);
+
+    Dart_Handle type = Dart_NewInteger(addr->GetType());
+    DART_CHECK_VALID_AND_PROPAGATE(type);
+    Dart_ListSetAt(entry, 0, type);

[zra, 2017/04/10 21:39:42]

Dart_ListSetAt returns a handle that needs to be checked for an error, as well.
For example, if the access is out-of-bounds.

[bkonyi, 2017/04/11 01:30:50]

Done.

+
+    Dart_Handle as_string = Dart_NewStringFromCString(addr->as_string());
+    DART_CHECK_VALID_AND_PROPAGATE(as_string);
+    Dart_ListSetAt(entry, 1, as_string);
+
+    RawAddr raw = addr->addr();
+    Dart_Handle data = SocketAddress::ToTypedData(raw);
+    DART_CHECK_VALID_AND_PROPAGATE(data);
+
+    Dart_ListSetAt(entry, 2, data);
+    Dart_ListSetAt(array, i, entry);
+  }
+  result = array;

[zra, 2017/04/10 21:39:43]

'result' doesn't appear to do very much. Maybe rename array -> result, or just
return array below.

[bkonyi, 2017/04/11 01:30:50]

Done.

+  delete addresses;
+  Dart_SetReturnValue(args, result);
+  return;
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_CreateConnectSync)(
+    Dart_NativeArguments args) {
+  RawAddr addr;
+  SocketAddress::GetSockAddr(Dart_GetNativeArgument(args, 1), &addr);
+  Dart_Handle port_arg = Dart_GetNativeArgument(args, 2);
+  DART_CHECK_VALID_AND_PROPAGATE(port_arg);
+  int64_t port = DartUtils::GetInt64ValueCheckRange(port_arg, 0, 65535);
+  SocketAddress::SetAddrPort(&addr, static_cast<intptr_t>(port));
+  intptr_t socket = SynchronousSocket::CreateConnect(addr);
+  if (socket >= 0) {
+    SynchronousSocket::SetSocketIdNativeField(Dart_GetNativeArgument(args, 0),
+                                              socket);
+    Dart_SetReturnValue(args, Dart_True());

[zra, 2017/04/10 21:39:43]

Dart_SetBooleanReturnValue(args, true);

[bkonyi, 2017/04/11 01:30:50]

Done.

+  } else {
+    OSError error;
+    Dart_SetReturnValue(args, DartUtils::NewDartOSError(&error));
+  }
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_WriteList)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  Dart_Handle buffer_obj = Dart_GetNativeArgument(args, 1);
+  if (!Dart_IsList(buffer_obj)) {
+    Dart_SetReturnValue(args, DartUtils::NewDartArgumentError(
+                                  "First parameter must be a List<int>"));
+    return;
+  }
+  intptr_t offset = DartUtils::GetIntptrValue(Dart_GetNativeArgument(args, 2));
+  intptr_t length = DartUtils::GetIntptrValue(Dart_GetNativeArgument(args, 3));
+  Dart_TypedData_Type type;
+  uint8_t* buffer = NULL;
+  intptr_t len;
+  Dart_Handle result = Dart_TypedDataAcquireData(
+      buffer_obj, &type, reinterpret_cast<void**>(&buffer), &len);
+  DART_CHECK_VALID_AND_PROPAGATE(result);
+  ASSERT((offset + length) <= len);
+  buffer += offset;
+  intptr_t bytes_written = SocketBase::Write(socket->fd(), buffer, length);
+  if (bytes_written >= 0) {
+    Dart_TypedDataReleaseData(buffer_obj);
+    Dart_SetReturnValue(args, Dart_NewInteger(bytes_written));

[zra, 2017/04/10 21:39:43]

Dart_SetIntegerReturnValue(args, bytes_written);

[bkonyi, 2017/04/11 01:30:50]

Done.

+  } else {
+    // Extract OSError before we release data, as it may override the error.
+    OSError os_error;
+    Dart_TypedDataReleaseData(buffer_obj);
+    Dart_SetReturnValue(args, DartUtils::NewDartOSError(&os_error));
+  }
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_ReadList)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  Dart_Handle buffer_obj = Dart_GetNativeArgument(args, 1);
+  if (!Dart_IsList(buffer_obj)) {
+    Dart_SetReturnValue(args, DartUtils::NewDartArgumentError(
+                                  "First parameter must be a List<int>"));
+    return;
+  }
+  intptr_t offset = DartUtils::GetIntptrValue(Dart_GetNativeArgument(args, 2));
+  intptr_t bytes = DartUtils::GetIntptrValue(Dart_GetNativeArgument(args, 3));
+  intptr_t array_len = 0;
+
+  Dart_Handle result = Dart_ListLength(buffer_obj, &array_len);
+  DART_CHECK_VALID_AND_PROPAGATE(result);
+

[zra, 2017/04/10 21:39:43]

Can we ASSERT something here about the list being long enough? There's a check
in the Dart code, but we should have it in both places to make sure no one
breaks the contract.

[bkonyi, 2017/04/11 01:30:50]

Do you mean check to see if the offset and bytes still fall within the valid
range?

[zra, 2017/04/11 16:19:40]

Yes. See the ASSERT in a similar location in File_ReadInto in
runtime/bin/file.cc

+  uint8_t* buffer = Dart_ScopeAllocate(bytes);
+  intptr_t bytes_read = SocketBase::Read(socket->fd(), buffer, bytes);
+  if (bytes_read >= 0) {
+    result = Dart_ListSetAsBytes(buffer_obj, offset, buffer, bytes_read);
+    if (Dart_IsError(result)) {
+      Dart_SetReturnValue(args, result);
+    } else {
+      Dart_SetReturnValue(args, Dart_NewInteger(bytes_read));

[zra, 2017/04/10 21:39:42]

Dart_SetIntegerReturnValue(args, bytes_read);

[bkonyi, 2017/04/11 01:30:50]

Done.

+    }
+  } else {
+    Dart_SetReturnValue(args, DartUtils::NewDartOSError());
+  }
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_Available)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  intptr_t available = SocketBase::Available(socket->fd());
+  if (available >= 0) {
+    Dart_SetReturnValue(args, Dart_NewInteger(available));

[zra, 2017/04/10 21:39:43]

Dart_SetIntegerReturnValue(args, available);

[bkonyi, 2017/04/11 01:30:50]

Done.

+  } else {
+    OSError os_error;
+    Dart_SetReturnValue(args, DartUtils::NewDartOSError(&os_error));
+  }
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_CloseSync)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  SocketBase::Close(socket->fd());
+  socket->SetClosedFd();
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_Read)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  int64_t length = 0;
+  if (DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 1), &length)) {

[zra, 2017/04/10 21:39:42]

if (!...) {
  // set error return
  return;
}

then un-nest.

[bkonyi, 2017/04/11 01:30:50]

Done.

+    uint8_t* buffer = NULL;
+    Dart_Handle result = IOBuffer::Allocate(length, &buffer);
+    DART_CHECK_VALID_AND_PROPAGATE(result);
+    ASSERT(buffer != NULL);
+    intptr_t bytes_read = SocketBase::Read(socket->fd(), buffer, length);
+    if (bytes_read == length) {
+      Dart_SetReturnValue(args, result);
+    } else if (bytes_read > 0) {
+      uint8_t* new_buffer = NULL;
+      Dart_Handle new_result = IOBuffer::Allocate(bytes_read, &new_buffer);
+      DART_CHECK_VALID_AND_PROPAGATE(new_result);
+      ASSERT(new_buffer != NULL);
+      memmove(new_buffer, buffer, bytes_read);
+      Dart_SetReturnValue(args, new_result);
+    } else if (bytes_read == 0) {
+      Dart_SetReturnValue(args, Dart_Null());

[zra, 2017/04/10 21:39:43]

I think Dart_Null() is already the default if nothing explicitly set.

[bkonyi, 2017/04/11 01:30:50]

Acknowledged.

+    } else {
+      ASSERT(bytes_read == -1);
+      Dart_SetReturnValue(args, DartUtils::NewDartOSError());
+    }
+  } else {
+    Dart_SetReturnValue(args, DartUtils::NewDartArgumentError(
+                                  "First parameter must be an integer."));
+    return;
+  }
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_ShutdownRead)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  SynchronousSocket::ShutdownRead(socket->fd());
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_ShutdownWrite)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  SynchronousSocket::ShutdownWrite(socket->fd());
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_GetPort)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  intptr_t port = SocketBase::GetPort(socket->fd());
+  if (port > 0) {
+    Dart_SetReturnValue(args, Dart_NewInteger(port));

[zra, 2017/04/10 21:39:43]

Dart_SetIntegerReturnValue(args, port)

[bkonyi, 2017/04/11 01:30:51]

Done.

+  } else {
+    Dart_SetReturnValue(args, DartUtils::NewDartOSError());
+  }
+}
+
+
+void FUNCTION_NAME(SynchronousSocket_GetRemotePeer)(Dart_NativeArguments args) {
+  SynchronousSocket* socket = SynchronousSocket::GetSocketIdNativeField(
+      Dart_GetNativeArgument(args, 0));
+  intptr_t port = 0;
+  SocketAddress* addr = SocketBase::GetRemotePeer(socket->fd(), &port);
+  if (addr == NULL) {
+    Dart_SetReturnValue(args, DartUtils::NewDartOSError());
+    return;
+  }
+  Dart_Handle list = Dart_NewList(2);
+  DART_CHECK_VALID_AND_PROPAGATE(list);

[zra, 2017/04/10 21:39:42]

This can leak 'addr'.

[bkonyi, 2017/04/11 01:30:50]

Fixed.

+
+  Dart_Handle entry = Dart_NewList(3);
+  DART_CHECK_VALID_AND_PROPAGATE(entry);
+
+  Dart_ListSetAt(entry, 0, Dart_NewInteger(addr->GetType()));

[zra, 2017/04/10 21:39:43]

Check the result of Dart_ListSetAt()

[bkonyi, 2017/04/11 01:30:51]

Done.

+  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;
+}
+
+
+static void SynchronousSocketFinalizer(void* isolate_data,
+                                       Dart_WeakPersistentHandle handle,
+                                       void* data) {
+  SynchronousSocket* socket = reinterpret_cast<SynchronousSocket*>(data);
+  if (socket->fd() >= 0) {
+    SocketBase::Close(socket->fd());
+    socket->SetClosedFd();
+  }
+  delete socket;
+}
+
+
+void SynchronousSocket::SetSocketIdNativeField(Dart_Handle handle,
+                                               intptr_t id) {
+  SynchronousSocket* socket = new SynchronousSocket(id);
+  Dart_Handle err = Dart_SetNativeInstanceField(
+      handle, kSocketIdNativeField, reinterpret_cast<intptr_t>(socket));
+  DART_CHECK_VALID_AND_PROPAGATE(err);

[zra, 2017/04/10 21:39:42]

This can leak 'socket'.

[bkonyi, 2017/04/11 01:30:51]

What would be the best way to deal with this case? We can't return an error to
Dart with this call in it's current state without propagating the error.

[zra, 2017/04/11 16:19:40]

There are two options:
1. Check that nothing is leaked from the callers of this function if you call
Dart_PropagateError() here.
2. Return the error handle, and check Dart_IsError() at the call sites.

(2) is probably safer, but we're also probably not consistent about it in other
places.

+  Dart_NewWeakPersistentHandle(handle, reinterpret_cast<void*>(socket),
+                               sizeof(SynchronousSocket),
+                               SynchronousSocketFinalizer);
+}
+
+
+SynchronousSocket* SynchronousSocket::GetSocketIdNativeField(
+    Dart_Handle socket_obj) {
+  intptr_t id;
+  Dart_Handle err =
+      Dart_GetNativeInstanceField(socket_obj, kSocketIdNativeField, &id);
+  DART_CHECK_VALID_AND_PROPAGATE(err);
+  SynchronousSocket* socket = reinterpret_cast<SynchronousSocket*>(id);
+  return socket;
+}
+
+}  // namespace bin
+}  // namespace dart
+
+#endif  // !defined(DART_IO_DISABLED)