serial: Check that port is open before reading or writing

This change adds checks to the platform-specific implementations
of Read() and Write() to make sure that the file descriptor is
valid before. This makes the assumptions validated by later DCHECK
correct.

This cannot be done in the platform-independent layer because test
code depends on being able to call some SerialIoHandler methods
without an actual file descriptor.

(cherry picked from commit adc731d)

(cherry picked from commit e07e479)

Bug: 1121836
Change-Id: If182404cf10a2f3b445b9c80b75fed5df6b5ab4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2393001
Reviewed-by: James Hollyer <jameshollyer@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#805016}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2406664
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Original-Commit-Position: refs/branch-heads/4240@{#632}
Cr-Original-Branched-From: f297677-refs/heads/master@{#800218}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2406151
Cr-Commit-Position: refs/branch-heads/4183@{#1813}
Cr-Branched-From: 740e9e8-refs/heads/master@{#782793}

services/device/serial/serial_io_handler_posix.cc

@@ -126,7 +126,11 @@ scoped_refptr<SerialIoHandler> SerialIoHandler::Create(
 void SerialIoHandlerPosix::ReadImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(pending_read_buffer());
-  DCHECK(file().IsValid());
+
+  if (!file().IsValid()) {
+    QueueReadCompleted(0, mojom::SerialReceiveError::DISCONNECTED);
+    return;
+  }
 
   // Try to read immediately. This is needed because on some platforms
   // (e.g., OSX) there may not be a notification from the message loop
@@ -138,7 +142,11 @@ void SerialIoHandlerPosix::ReadImpl() {
 void SerialIoHandlerPosix::WriteImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(pending_write_buffer());
-  DCHECK(file().IsValid());
+
+  if (!file().IsValid()) {
+    QueueWriteCompleted(0, mojom::SerialSendError::DISCONNECTED);
+    return;
+  }
 
   EnsureWatchingWrites();
 }

services/device/serial/serial_io_handler_win.cc

@@ -266,7 +266,11 @@ bool SerialIoHandlerWin::PostOpen() {
 void SerialIoHandlerWin::ReadImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(pending_read_buffer());
-  DCHECK(file().IsValid());
+
+  if (!file().IsValid()) {
+    QueueReadCompleted(0, mojom::SerialReceiveError::DISCONNECTED);
+    return;
+  }
 
   if (!SetCommMask(file().GetPlatformFile(), EV_RXCHAR)) {
     VPLOG(1) << "Failed to set serial event flags";
@@ -285,7 +289,11 @@ void SerialIoHandlerWin::ReadImpl() {
 void SerialIoHandlerWin::WriteImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(pending_write_buffer());
-  DCHECK(file().IsValid());
+
+  if (!file().IsValid()) {
+    QueueWriteCompleted(0, mojom::SerialSendError::DISCONNECTED);
+    return;
+  }
 
   BOOL ok = ::WriteFile(file().GetPlatformFile(), pending_write_buffer(),
                         pending_write_buffer_len(), NULL,

services/device/serial/serial_port_impl_unittest.cc

@@ -21,9 +21,65 @@ class SerialPortImplTest : public DeviceServiceTestBase {
   ~SerialPortImplTest() override = default;
 
  protected:
+  void CreateDataPipe(mojo::ScopedDataPipeProducerHandle* producer,
+                      mojo::ScopedDataPipeConsumerHandle* consumer) {
+    MojoCreateDataPipeOptions options;
+    options.struct_size = sizeof(MojoCreateDataPipeOptions);
+    options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
+    options.element_num_bytes = 1;
+    options.capacity_num_bytes = 64;
+
+    MojoResult result = mojo::CreateDataPipe(&options, producer, consumer);
+    DCHECK_EQ(result, MOJO_RESULT_OK);
+  }
+
+  mojo::ScopedDataPipeConsumerHandle StartReading(
+      mojom::SerialPort* serial_port) {
+    mojo::ScopedDataPipeProducerHandle producer;
+    mojo::ScopedDataPipeConsumerHandle consumer;
+    CreateDataPipe(&producer, &consumer);
+    serial_port->StartReading(std::move(producer));
+    return consumer;
+  }
+
+  mojo::ScopedDataPipeProducerHandle StartWriting(
+      mojom::SerialPort* serial_port) {
+    mojo::ScopedDataPipeProducerHandle producer;
+    mojo::ScopedDataPipeConsumerHandle consumer;
+    CreateDataPipe(&producer, &consumer);
+    serial_port->StartWriting(std::move(consumer));
+    return producer;
+  }
+
   DISALLOW_COPY_AND_ASSIGN(SerialPortImplTest);
 };
 
+TEST_F(SerialPortImplTest, StartIoBeforeOpen) {
+  mojo::Remote<mojom::SerialPort> serial_port;
+  mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher_remote;
+  mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher =
+      mojo::MakeSelfOwnedReceiver(
+          std::make_unique<mojom::SerialPortConnectionWatcher>(),
+          watcher_remote.InitWithNewPipeAndPassReceiver());
+  SerialPortImpl::Create(
+      base::FilePath(FILE_PATH_LITERAL("/dev/fakeserialmojo")),
+      serial_port.BindNewPipeAndPassReceiver(), std::move(watcher_remote),
+      task_environment_.GetMainThreadTaskRunner());
+
+  mojo::ScopedDataPipeConsumerHandle consumer = StartReading(serial_port.get());
+  mojo::ScopedDataPipeProducerHandle producer = StartWriting(serial_port.get());
+
+  // Write some data so that StartWriting() will cause a call to Write().
+  static const char kBuffer[] = "test";
+  uint32_t bytes_written = base::size(kBuffer);
+  MojoResult result =
+      producer->WriteData(&kBuffer, &bytes_written, MOJO_WRITE_DATA_FLAG_NONE);
+  DCHECK_EQ(result, MOJO_RESULT_OK);
+  DCHECK_EQ(bytes_written, base::size(kBuffer));
+
+  base::RunLoop().RunUntilIdle();
+}
+
 TEST_F(SerialPortImplTest, WatcherClosedWhenPortClosed) {
   mojo::Remote<mojom::SerialPort> serial_port;
   mojo::PendingRemote<mojom::SerialPortConnectionWatcher> watcher;