commit 4e887611f9d73ea1fa7018ccecac73b65f9a78e9
author Martin Lindsay <mlindsay@google.com> Wed May 26 18:11:39 2021 +0000
committer releases-try-builder <releases-try-builder@fuchsia-infra.iam.gserviceaccount.com> Sat Jun 05 03:51:22 2021 +0000
tree 7bccd1395ad2bcea8481822001210aaed00af8cb
parent 2d7e986dafb864ed34817554adbc57e443c6e646

[aml-rawnand] Fix nullptr checks for out variables while reading.

Additionally adds some smoke testing calls into the unit tests to ensure
that the right handling is used for when nullptr is passed.

Bug: 77493, b/179400686
Change-Id: I52f6c3fdce98b067d06bd65079b316ceec022c71
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/535246
Reviewed-by: Chris Drouillard <cdrllrd@google.com>
Commit-Queue: Martin Lindsay <mlindsay@google.com>

src/devices/nand/drivers/aml-rawnand/aml-rawnand.cc

diff --git a/src/devices/nand/drivers/aml-rawnand/aml-rawnand.cc b/src/devices/nand/drivers/aml-rawnand/aml-rawnand.cc
index 24acd89..2a69092 100644
--- a/src/devices/nand/drivers/aml-rawnand/aml-rawnand.cc
+++ b/src/devices/nand/drivers/aml-rawnand/aml-rawnand.cc
@@ -575,11 +575,6 @@
     return status;
   }
 
-  if (oob != nullptr && AmlGetOOBByte(reinterpret_cast<uint8_t*>(oob), oob_actual) != ZX_OK &&
-      oob_actual != nullptr) {
-    *oob_actual = 0;
-  }
-
   status = AmlGetECCCorrections(ecc_pages, nand_page, ecc_correct, &erased);
   if (status != ZX_OK) {
     zxlogf(WARNING, "%s: Uncorrectable ECC error on read", __func__);
@@ -587,13 +582,25 @@
   }
 
   // Finally copy out the data and oob as needed.
+  if (oob != nullptr) {
+    size_t num_bytes;
+    // Need to try to fetch it first, just to get the oob_actual size
+    if (AmlGetOOBByte(reinterpret_cast<uint8_t*>(oob), &num_bytes) != ZX_OK) {
+      num_bytes = 0;
+    }
+    if (erased) {
+      memset(oob, 0xff, num_bytes);
+    }
+    if (oob_actual != nullptr) {
+      *oob_actual = num_bytes;
+    }
+  }
   if (data != nullptr) {
     // Page0 is always 384 bytes.
     size_t num_bytes = (page0 ? 384 : writesize_);
     // Clean up any possible bit flips on supposed erased pages.
     if (erased) {
       memset(data, 0xff, num_bytes);
-      memset(oob, 0xff, *oob_actual);
     } else {
       memcpy(data, buffers_->data_buf, num_bytes);
     }

src/devices/nand/drivers/aml-rawnand/tests/aml-rawnand-test.cc

diff --git a/src/devices/nand/drivers/aml-rawnand/tests/aml-rawnand-test.cc b/src/devices/nand/drivers/aml-rawnand/tests/aml-rawnand-test.cc
index e69eb43..b36f3fb 100644
--- a/src/devices/nand/drivers/aml-rawnand/tests/aml-rawnand-test.cc
+++ b/src/devices/nand/drivers/aml-rawnand/tests/aml-rawnand-test.cc
@@ -503,6 +503,21 @@
   EXPECT_EQ(0xff, data.back());
   EXPECT_EQ(0xffff, oob.front());
   EXPECT_EQ(0xffff, oob.back());
+
+  // Repeat read with various nullptr combinations to ensure no crash.
+  ASSERT_OK(nand->RawNandReadPageHwecc(5, nullptr, kTestNandWriteSize, &data_bytes_read,
+                                       reinterpret_cast<uint8_t*>(oob.data()), kDefaultNumUserBytes,
+                                       &oob_bytes_read, &ecc_correct));
+  ASSERT_OK(nand->RawNandReadPageHwecc(5, data.data(), kTestNandWriteSize, &data_bytes_read,
+                                       nullptr, kDefaultNumUserBytes, &oob_bytes_read,
+                                       &ecc_correct));
+  ASSERT_OK(nand->RawNandReadPageHwecc(5, nullptr, kTestNandWriteSize, &data_bytes_read, nullptr,
+                                       kDefaultNumUserBytes, &oob_bytes_read, &ecc_correct));
+  ASSERT_OK(nand->RawNandReadPageHwecc(5, nullptr, kTestNandWriteSize, nullptr,
+                                       reinterpret_cast<uint8_t*>(oob.data()), kDefaultNumUserBytes,
+                                       nullptr, &ecc_correct));
+  ASSERT_OK(nand->RawNandReadPageHwecc(5, nullptr, kTestNandWriteSize, nullptr, nullptr,
+                                       kDefaultNumUserBytes, nullptr, &ecc_correct));
 }
 
 TEST(AmlRawnand, PartialErasedPage) {