pw_kvs: Check for values that are too large
- Reject values that are too large to fit in one sector.
- Clean up KeyDescriptor usage.
Change-Id: I8575bd0f3b7e9c6c2a5c5e9ef5ba1bc0dd81da7b
diff --git a/pw_kvs/key_value_store.cc b/pw_kvs/key_value_store.cc
index b18d4f2..98a31d1 100644
--- a/pw_kvs/key_value_store.cc
+++ b/pw_kvs/key_value_store.cc
@@ -43,8 +43,7 @@
entry_header_format_(format),
options_(options),
sectors_(partition_.sector_count()),
- last_new_sector_(sectors_.data()),
- working_buffer_{} {}
+ last_new_sector_(sectors_.data()) {}
Status KeyValueStore::Init() {
if (kMaxUsableSectors < sectors_.size()) {
@@ -186,36 +185,26 @@
// With the new key descriptor, either add it to the descriptor table or
// overwrite an existing entry with an older version of the key.
KeyDescriptor* existing_descriptor = FindDescriptor(key_descriptor.key_hash);
- if (existing_descriptor) {
- if (existing_descriptor->key_version < key_descriptor.key_version) {
- // Existing entry is old; replace the existing entry with the new one.
- *existing_descriptor = key_descriptor;
- } else {
- // Otherwise, check for data integrity and leave the existing entry.
- if (existing_descriptor->key_version == key_descriptor.key_version) {
- ERR("Data loss: Duplicated old(=%zu) and new(=%zu) version",
- size_t(existing_descriptor->key_version),
- size_t(key_descriptor.key_version));
- return Status::DATA_LOSS;
- }
- DBG("Found stale entry when appending; ignoring");
- }
- return Status::OK;
- }
- // Write new entry.
- KeyDescriptor* newly_allocated_key_descriptor;
- TRY(AppendEmptyDescriptor(&newly_allocated_key_descriptor));
- *newly_allocated_key_descriptor = key_descriptor;
- return Status::OK;
-}
-// TODO: Need a better name.
-Status KeyValueStore::AppendEmptyDescriptor(KeyDescriptor** new_descriptor) {
- if (key_descriptors_.full()) {
- return Status::RESOURCE_EXHAUSTED;
+ // Write a new entry.
+ if (existing_descriptor == nullptr) {
+ if (key_descriptors_.full()) {
+ return Status::RESOURCE_EXHAUSTED;
+ }
+ key_descriptors_.push_back(key_descriptor);
+ } else if (existing_descriptor->key_version < key_descriptor.key_version) {
+ // Existing entry is old; replace the existing entry with the new one.
+ *existing_descriptor = key_descriptor;
+ } else {
+ // Otherwise, check for data integrity and leave the existing entry.
+ if (existing_descriptor->key_version == key_descriptor.key_version) {
+ ERR("Data loss: Duplicated old(=%zu) and new(=%zu) version",
+ size_t(existing_descriptor->key_version),
+ size_t(key_descriptor.key_version));
+ return Status::DATA_LOSS;
+ }
+ DBG("Found stale entry when appending; ignoring");
}
- key_descriptors_.emplace_back();
- *new_descriptor = &key_descriptors_.back();
return Status::OK;
}
@@ -262,8 +251,11 @@
TRY(CheckOperation(key));
- if (value.size() > (1 << 24)) {
- // TODO: Reject sizes that are larger than the maximum?
+ if (Entry::size(partition_, key, value) > partition_.sector_size_bytes()) {
+ DBG("%zu B value with %zu B key cannot fit in one sector",
+ value.size(),
+ key.size());
+ return Status::INVALID_ARGUMENT;
}
KeyDescriptor* key_descriptor;
@@ -450,8 +442,8 @@
SectorDescriptor* old_sector = SectorFromAddress(key_descriptor->address);
SectorDescriptor* sector;
- TRY(FindOrRecoverSectorWithSpace(
- §or, Entry::size(partition_.alignment_bytes(), key, value)));
+ TRY(FindOrRecoverSectorWithSpace(§or,
+ Entry::size(partition_, key, value)));
DBG("Writing existing entry; found sector: %zu", SectorIndex(sector));
if (old_sector != SectorFromAddress(key_descriptor->address)) {
@@ -482,8 +474,8 @@
KeyDescriptor key_descriptor(key, 0, 0);
SectorDescriptor* sector;
- TRY(FindOrRecoverSectorWithSpace(
- §or, Entry::size(partition_.alignment_bytes(), key, value)));
+ TRY(FindOrRecoverSectorWithSpace(§or,
+ Entry::size(partition_, key, value)));
DBG("Writing new entry; found sector: %zu", SectorIndex(sector));
TRY(AppendEntry(sector, &key_descriptor, key, value, KeyDescriptor::kValid));
diff --git a/pw_kvs/key_value_store_test.cc b/pw_kvs/key_value_store_test.cc
index 1644e7c..0194b81 100644
--- a/pw_kvs/key_value_store_test.cc
+++ b/pw_kvs/key_value_store_test.cc
@@ -282,6 +282,22 @@
}
}
+TEST_F(EmptyInitializedKvs, Put_MaxValueSize) {
+ size_t max_value_size =
+ test_partition.sector_size_bytes() - sizeof(EntryHeader) - 1;
+
+ // Use the large_test_flash as a big chunk of data for the Put statement.
+ ASSERT_GT(sizeof(large_test_flash), max_value_size + 2 * sizeof(EntryHeader));
+ auto big_data = as_bytes(span(&large_test_flash, 1));
+
+ EXPECT_EQ(Status::OK, kvs_.Put("K", big_data.subspan(0, max_value_size)));
+
+ // Larger than maximum is rejected.
+ EXPECT_EQ(Status::INVALID_ARGUMENT,
+ kvs_.Put("K", big_data.subspan(0, max_value_size + 1)));
+ EXPECT_EQ(Status::INVALID_ARGUMENT, kvs_.Put("K", big_data));
+}
+
TEST_F(EmptyInitializedKvs, Delete_GetDeletedKey_ReturnsNotFound) {
ASSERT_EQ(Status::OK, kvs_.Put("kEy", as_bytes(span("123"))));
ASSERT_EQ(Status::OK, kvs_.Delete("kEy"));
diff --git a/pw_kvs/public/pw_kvs/key_value_store.h b/pw_kvs/public/pw_kvs/key_value_store.h
index b47041e..72dd1a4 100644
--- a/pw_kvs/public/pw_kvs/key_value_store.h
+++ b/pw_kvs/public/pw_kvs/key_value_store.h
@@ -236,8 +236,6 @@
struct KeyDescriptor {
enum State { kValid, kDeleted };
- KeyDescriptor() = default;
-
KeyDescriptor(std::string_view key,
uint32_t version,
Address addr,
@@ -401,7 +399,7 @@
// Working buffer is a general purpose buffer for operations (such as init or
// relocate) to use for working space to remove the need to allocate temporary
// space.
- std::array<char, kWorkingBufferSizeBytes> working_buffer_;
+ std::array<std::byte, kWorkingBufferSizeBytes> working_buffer_;
};
} // namespace pw::kvs
diff --git a/pw_kvs/pw_kvs_private/entry.h b/pw_kvs/pw_kvs_private/entry.h
index 4cae4bb..3741ea1 100644
--- a/pw_kvs/pw_kvs_private/entry.h
+++ b/pw_kvs/pw_kvs_private/entry.h
@@ -15,6 +15,7 @@
// This file defines classes for managing the in-flash format for KVS entires.
#pragma once
+#include <algorithm>
#include <array>
#include <cstddef>
#include <cstdint>
@@ -60,7 +61,6 @@
public:
static constexpr size_t kMinAlignmentBytes = sizeof(EntryHeader);
static constexpr size_t kMaxKeyLength = 0b111111;
- static constexpr size_t kMaxValueSize = 0xFFFF;
using Address = FlashPartition::Address;
@@ -144,11 +144,11 @@
Status VerifyChecksumInFlash(ChecksumAlgorithm* algorithm) const;
// Calculates the total size of an entry, including padding.
- static constexpr size_t size(size_t alignment_bytes,
- std::string_view key,
- span<const std::byte> value) {
+ static size_t size(const FlashPartition& partition,
+ std::string_view key,
+ span<const std::byte> value) {
return AlignUp(sizeof(EntryHeader) + key.size() + value.size(),
- alignment_bytes);
+ std::max(partition.alignment_bytes(), kMinAlignmentBytes));
}
// The address at which the next possible entry could be located.
