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.

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-Commit-Position: refs/heads/master@{#805016}

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

@@ -168,7 +168,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;
+  }
 
   ClearPendingError();
   if (!IsReadPending())
@@ -185,7 +189,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;
+  }
 
   if (!WriteFile(file().GetPlatformFile(), pending_write_buffer(),
                  pending_write_buffer_len(), nullptr,

services/device/serial/serial_port_impl_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "services/device/serial/serial_port_impl.h"
 
+#include "base/stl_util.h"
 #include "base/test/bind_test_util.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -94,8 +95,52 @@ class SerialPortImplTest : public DeviceServiceTestBase {
     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;
+  }
 };
 
+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::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher;
@@ -139,10 +184,7 @@ TEST_F(SerialPortImplTest, FlushRead) {
   mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher;
   CreatePort(&serial_port, &watcher);
 
-  mojo::ScopedDataPipeProducerHandle producer;
-  mojo::ScopedDataPipeConsumerHandle consumer;
-  CreateDataPipe(&producer, &consumer);
-  serial_port->StartReading(std::move(producer));
+  mojo::ScopedDataPipeConsumerHandle consumer = StartReading(serial_port.get());
 
   // Calling Flush(kReceive) should cause the data pipe to close.
   base::RunLoop watcher_loop;
@@ -170,10 +212,7 @@ TEST_F(SerialPortImplTest, FlushWrite) {
   mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher;
   CreatePort(&serial_port, &watcher);
 
-  mojo::ScopedDataPipeProducerHandle producer;
-  mojo::ScopedDataPipeConsumerHandle consumer;
-  CreateDataPipe(&producer, &consumer);
-  serial_port->StartWriting(std::move(consumer));
+  mojo::ScopedDataPipeProducerHandle producer = StartWriting(serial_port.get());
 
   // Calling Flush(kTransmit) should cause the data pipe to close.
   base::RunLoop watcher_loop;
@@ -201,10 +240,7 @@ TEST_F(SerialPortImplTest, Drain) {
   mojo::SelfOwnedReceiverRef<mojom::SerialPortConnectionWatcher> watcher;
   CreatePort(&serial_port, &watcher);
 
-  mojo::ScopedDataPipeProducerHandle producer;
-  mojo::ScopedDataPipeConsumerHandle consumer;
-  CreateDataPipe(&producer, &consumer);
-  serial_port->StartWriting(std::move(consumer));
+  mojo::ScopedDataPipeProducerHandle producer = StartWriting(serial_port.get());
 
   // Drain() will wait for the data pipe to close before replying.
   producer.reset();