bugfix: Fix invalid data handling in deserialization
When loading from sysfile, it is possible that the 'size'
read is junk data when there was no persisted blobs, and is too large causing
'length_error' or 'bad_alloc' to be raised. Improve handling by
1) Returning empty string if 'size' specified is 0 or too large.
2) Ignoring all but system_error which is thrown from sysfile read operation.
3) Not commiting the empty blob when no persisted data is found.
4) Adding more unit tests.
Resolves openbmc/phosphor-ipmi-blobs-binarystore#1
Signed-off-by: Kun Yi <kunyi731@gmail.com>
Change-Id: Iee16cf5254242856efe6bcca59ef2ca7c4f09c7c
diff --git a/binarystore.cpp b/binarystore.cpp
index a18a980..1c94dd3 100644
--- a/binarystore.cpp
+++ b/binarystore.cpp
@@ -74,15 +74,22 @@
log<level::WARNING>(
"Fail to parse. There might be no persisted blobs",
entry("BASE_ID=%s", baseBlobId_.c_str()));
+
+ return true;
}
}
- catch (const std::exception& e)
+ catch (const std::system_error& e)
{
/* Read causes unexpected system-level failure */
log<level::ERR>("Reading from sysfile failed",
entry("ERROR=%s", e.what()));
return false;
}
+ catch (const std::exception& e)
+ {
+ log<level::WARNING>("Invalid size. There might be no persisted blobs.");
+ return true;
+ }
if (blob_.blob_base_id() != baseBlobId_)
{
diff --git a/sys_file.cpp b/sys_file.cpp
index 2336af2..92d6c85 100644
--- a/sys_file.cpp
+++ b/sys_file.cpp
@@ -80,6 +80,13 @@
std::string SysFileImpl::readAsStr(size_t pos, size_t count) const
{
std::string result;
+
+ /* If count is invalid, return an empty string. */
+ if (count == 0 || count > result.max_size())
+ {
+ return result;
+ }
+
result.resize(count);
size_t bytesRead = readToBuf(pos, count, result.data());
result.resize(bytesRead);
diff --git a/sys_file.hpp b/sys_file.hpp
index 5a2bb85..63110d2 100644
--- a/sys_file.hpp
+++ b/sys_file.hpp
@@ -34,8 +34,10 @@
* @param pos The byte pos into the file to read from
* @param count How many bytes to read
* @returns The data read in a vector, whose size might be smaller than
- * count if there is not enough to read.
+ * count if there is not enough to read. Might be empty if the
+ * count specified is too large to even fit in a std::string.
* @throws std::system_error if read operation cannot be completed
+ * std::bad_alloc if cannot construct string with 'count' size
*/
virtual std::string readAsStr(size_t pos, size_t count) const = 0;
diff --git a/test/handler_unittest.cpp b/test/handler_unittest.cpp
index 31ccdc6..b6d204e 100644
--- a/test/handler_unittest.cpp
+++ b/test/handler_unittest.cpp
@@ -13,6 +13,8 @@
#include "binaryblob.pb.h"
+#include <gtest/gtest.h>
+
using ::testing::_;
using ::testing::AtLeast;
using ::testing::ElementsAreArray;
@@ -178,4 +180,25 @@
EXPECT_EQ(handler.getBlobIds(), expectedIdList);
}
+TEST_F(BinaryStoreBlobHandlerBasicTest, CreatingFromEmptySysfile)
+{
+ const std::string emptyData;
+ EXPECT_NO_THROW(handler.addNewBinaryStore(BinaryStore::createFromConfig(
+ basicTestBaseId, std::make_unique<FakeSysFile>(emptyData), 0)));
+ EXPECT_TRUE(handler.canHandleBlob(basicTestBlobId));
+}
+
+TEST_F(BinaryStoreBlobHandlerBasicTest, CreatingFromJunkData)
+{
+ boost::endian::little_uint64_t tooLarge = 0xffffffffffffffffull;
+ const std::string junkDataWithLargeSize(tooLarge.data(), sizeof(tooLarge));
+ EXPECT_GE(tooLarge, junkDataWithLargeSize.max_size());
+
+ EXPECT_NO_THROW(handler.addNewBinaryStore(BinaryStore::createFromConfig(
+ basicTestBaseId, std::make_unique<FakeSysFile>(junkDataWithLargeSize),
+ 0)));
+
+ EXPECT_TRUE(handler.canHandleBlob(basicTestBlobId));
+}
+
} // namespace blobs