commit c042abbb75ed3f1977a13a0c6e10da1dabe79609
author Nicholas Bishop <nbishop@neverware.com> Fri Jul 03 12:46:58 2020 -0400
committer Commit Bot <commit-bot@chromium.org> Wed Jul 29 20:50:35 2020 +0000
tree 2d078da418ebcbb186f6e0b264f670e0569035fa
parent 3335cd5e412e2748a477e49c7f649ff4ac42231c

login: fix check for file descriptor validity

LoginScreenStorage was incorrectly checking a file descriptor against
zero, but it should check against -1. Fixed by using
ScopedFD::is_valid instead of ScopedFD::get.

Added a new RetrieveInvalidData test that verifies that Retrieve
properly returns false if the file associated with the key is
empty. This required modifying
FakeSharedMemoryUtil::WriteDataToSharedMemory to return an invalid
file descriptor if the input vector is empty, which matches the
behavior of base::SharedMemory::Create.

BUG=b:159942676
TEST=cros_workon_make --test chromeos-login

Change-Id: I4b33b54426880f47b297c2e4d431ed7a608fdce0
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2281102
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Tested-by: Alexander Hendrich <hendrich@chromium.org>
Tested-by: Nicholas Bishop <nbishop@neverware.com>
Commit-Queue: Alexander Hendrich <hendrich@chromium.org>

login_manager/fake_secret_util.cc

diff --git a/login_manager/fake_secret_util.cc b/login_manager/fake_secret_util.cc
index 35b506c..e65aa36 100644
--- a/login_manager/fake_secret_util.cc
+++ b/login_manager/fake_secret_util.cc
@@ -12,6 +12,12 @@
 
 base::ScopedFD FakeSharedMemoryUtil::WriteDataToSharedMemory(
     const std::vector<uint8_t>& data) {
+  // Return an invalid file descriptor if the input is empty to match
+  // the behavior of base::SharedMemory::Create.
+  if (data.empty()) {
+    return base::ScopedFD();
+  }
+
   base::ScopedFD fd(open("/dev/null", O_RDONLY));
   data_[fd.get()] = data;
   return fd;

login_manager/login_screen_storage.cc

diff --git a/login_manager/login_screen_storage.cc b/login_manager/login_screen_storage.cc
index 0e80c75..42cde6e 100644
--- a/login_manager/login_screen_storage.cc
+++ b/login_manager/login_screen_storage.cc
@@ -161,7 +161,7 @@
     const std::vector<uint8_t>& data,
     base::ScopedFD* out_shared_memory_fd) {
   *out_shared_memory_fd = shared_memory_util_->WriteDataToSharedMemory(data);
-  if (!out_shared_memory_fd->get()) {
+  if (!out_shared_memory_fd->is_valid()) {
     *error = CreateError(DBUS_ERROR_IO_ERROR, "couldn't create shared memory.");
     return false;
   }

login_manager/login_screen_storage_test.cc

diff --git a/login_manager/login_screen_storage_test.cc b/login_manager/login_screen_storage_test.cc
index 4fabb0c..6c109f8 100644
--- a/login_manager/login_screen_storage_test.cc
+++ b/login_manager/login_screen_storage_test.cc
@@ -170,6 +170,27 @@
         testing::Values(std::vector<uint8_t>{0xb1, 0x0b},
                         GenerateLongTestValue())));
 
+TEST_F(LoginScreenStorageTestBase, RetrieveInvalidData) {
+  const base::FilePath path = GetKeyPath(kTestKey);
+
+  // Make the storage subdirectory
+  EXPECT_TRUE(base::CreateDirectory(path.DirName()));
+
+  // Create an empty file
+  base::ScopedFILE file(base::OpenFile(GetKeyPath(kTestKey), "w"));
+  EXPECT_NE(file, nullptr);
+  file.reset();
+
+  brillo::ErrorPtr error;
+  base::ScopedFD out_value_fd;
+  uint64_t value_size;
+  // CreateSharedMemoryWithData should fail because it can't create
+  // zero-sized shared memory, check that Retrieve propagates that
+  // failure.
+  EXPECT_FALSE(
+      storage_->Retrieve(&error, kTestKey, &value_size, &out_value_fd));
+}
+
 class LoginScreenStorageTestPersistent : public LoginScreenStorageTestBase {
  protected:
   const std::vector<uint8_t> test_value_{0xb1, 0x0b};