Skip to content

Commit

Permalink
Fix PersistentStorageDelegate 0-storage behavior (#16139)
Browse files Browse the repository at this point in the history
* Fix PersistentStorageDelegate 0-storage behavior

- Make Server's PersistentStorageDelegate and
  TestPersistentStorage delegate properly handle nullptr
  input for zero-length.
- Document behavior expected of PersistentStorageDelegate interface

Issue #16130

* Add unit tests for TestPersistentStorageDelegate

* Address review comments

* Restyled by clang-format

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
  • Loading branch information
3 people authored Mar 14, 2022
1 parent b8c5d51 commit 1f0307d
Show file tree
Hide file tree
Showing 7 changed files with 352 additions and 48 deletions.
34 changes: 33 additions & 1 deletion src/app/server/Server.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,19 +115,51 @@ class Server
{
CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
{
uint8_t emptyPlaceholder = 0;
if (buffer == nullptr)
{
if (size != 0)
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
else
{
// When size is zero, let's give a non-nullptr to the KVS backend
buffer = &emptyPlaceholder;
}
}

size_t bytesRead = 0;
CHIP_ERROR err = DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size, &bytesRead);

// Update size only if it made sense
if ((CHIP_ERROR_BUFFER_TOO_SMALL == err) || (CHIP_NO_ERROR == err))
{
size = CanCastTo<uint16_t>(bytesRead) ? static_cast<uint16_t>(bytesRead) : 0;
}

if (err == CHIP_NO_ERROR)
{
ChipLogProgress(AppServer, "Retrieved from server storage: %s", key);
}
size = static_cast<uint16_t>(bytesRead);

return err;
}

CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override
{
uint8_t placeholderForEmpty = 0;
if (value == nullptr)
{
if (size == 0)
{
value = &placeholderForEmpty;
}
else
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
}
return DeviceLayer::PersistedStorage::KeyValueStoreMgr().Put(key, value, size);
}

Expand Down
12 changes: 6 additions & 6 deletions src/app/tests/TestBindingTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,26 +150,26 @@ void TestPersistentStorage(nlTestSuite * aSuite, void * aContext)
VerifyRestored(aSuite, testStorage, expected);

// Verify storage untouched if add fails
testStorage.AddErrorKey(key.BindingTableEntry(4));
testStorage.AddPoisonKey(key.BindingTableEntry(4));
NL_TEST_ASSERT(aSuite, table.Add(EmberBindingTableEntry::ForNode(4, 4, 0, 0, NullOptional)) != CHIP_NO_ERROR);
VerifyRestored(aSuite, testStorage, expected);
testStorage.ClearErrorKey();
testStorage.ClearPoisonKeys();

// Verify storage untouched if removing head fails
testStorage.AddErrorKey(key.BindingTable());
testStorage.AddPoisonKey(key.BindingTable());
auto iter = table.begin();
NL_TEST_ASSERT(aSuite, table.RemoveAt(iter) != CHIP_NO_ERROR);
VerifyTableSame(aSuite, table, expected);
testStorage.ClearErrorKey();
testStorage.ClearPoisonKeys();
VerifyRestored(aSuite, testStorage, expected);

// Verify storage untouched if removing other nodes fails
testStorage.AddErrorKey(key.BindingTableEntry(0));
testStorage.AddPoisonKey(key.BindingTableEntry(0));
iter = table.begin();
++iter;
NL_TEST_ASSERT(aSuite, table.RemoveAt(iter) != CHIP_NO_ERROR);
VerifyTableSame(aSuite, table, expected);
testStorage.ClearErrorKey();
testStorage.ClearPoisonKeys();
VerifyRestored(aSuite, testStorage, expected);

// Verify removing head
Expand Down
64 changes: 41 additions & 23 deletions src/lib/core/CHIPPersistentStorageDelegate.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
* Copyright (c) 2020-2022 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -18,14 +18,22 @@

#pragma once

#include <lib/core/CHIPCore.h>
#include <lib/core/CHIPError.h>
#include <lib/support/DLLUtil.h>
#include <stddef.h>
#include <stdint.h>

namespace chip {

class DLL_EXPORT PersistentStorageDelegate
{
public:
/**
* Maximum length of a key required by implementations. Any implementer of PersistentStorageDelegate
* must support keys AT LEAST this long.
*/
static constexpr size_t kKeyLengthMax = 32;

virtual ~PersistentStorageDelegate() {}

/**
Expand All @@ -38,35 +46,44 @@ class DLL_EXPORT PersistentStorageDelegate
* Caller is responsible to take care of any special formatting needs (e.g. byte
* order, null terminators, consistency checks or versioning).
*
* This API allows for determining the size of a stored value. Whenever
* the passed `size` is smaller than needed and the key exists in storage, the error
* CHIP_ERROR_BUFFER_TOO_SMALL will be given, and the `size` will be updated to the
* size of the stored value. It is legal to use `nullptr` for `buffer` if `size` is 0.
*
* If a key is found and the `buffer`'s `size` is large enough, then the value will
* be copied to `buffer` and `size` will be updated to the actual size used.
*
* The easiest way to determine if a key exists (and the value's size if so) is to pass
* `size` of 0, which is always valid to do, and will return CHIP_ERROR_BUFFER_TOO_SMALL
* if the key exists and CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if the
* key is not found.
*
* @param[in] key Key to lookup
* @param[out] buffer Value for the key
* @param[in, out] size Input value buffer size, output length of value.
* The output length could be larger than input value. In
* such cases, the user should allocate the buffer large
* enough (>= output length), and call the API again.
* @param[out] buffer Pointer to a buffer where the place the read value.
* @param[in, out] size Input is maximum buffer size, output updated to length of value.
*
* @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is unknown.
* @return CHIP_ERROR_BUFFER_TOO_SMALL the provided buffer is not big
* enough. In this case "size" will
* indicate the needed buffer size.
* Some data may or may not be placed
* in "buffer" in this case; consumers
* should not rely on that behavior.
* CHIP_ERROR_BUFFER_TOO_SMALL combined
* with setting "size" to 0 means the
* actual size was too large to fit in
* uint16_t.
* @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is not found in storage.
* @return CHIP_ERROR_BUFFER_TOO_SMALL the provided buffer is not big enough. In this case
* "size" will indicate the needed buffer size. Some data
* may or may not be placed in "buffer" in this case; consumers
* should not rely on that behavior. CHIP_ERROR_BUFFER_TOO_SMALL
* combined with setting "size" to 0 means the actual size was
* too large to fit in uint16_t.
*/
virtual CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) = 0;

/**
* @brief
* Set the value for the key to a byte buffer.
* Set the value for the key to a byte buffer. Empty values can be stored
* with size == 0, in which case `value` may be nullptr.
*
* @param[in] key Key to set
* @param[in] value Pointer to bytes of value to be set. `value` can only be `nullptr` if size == 0.
* @param[in] size Size of the `value` to store.
*
* @param[in] key Key to be set
* @param[in] value Value to be set
* @param[in] size Size of the Value
* @return CHIP_NO_ERROR on success, CHIP_INVALID_ARGUMENT on `value` being `nullptr` with size > 0,
* or another CHIP_ERROR value from implementation on failure.
*/
virtual CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) = 0;

Expand All @@ -76,7 +93,8 @@ class DLL_EXPORT PersistentStorageDelegate
*
* @param[in] key Key to be deleted
*
* @return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is unknown.
* @return CHIP_NO_ERROR on success, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is not found in storage,
* or another CHIP_ERROR value from implementation on failure.
*/
virtual CHIP_ERROR SyncDeleteKeyValue(const char * key) = 0;
};
Expand Down
5 changes: 2 additions & 3 deletions src/lib/support/DefaultStorageKeyAllocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include <app/ConcreteAttributePath.h>
#include <app/util/basic-types.h>
#include <lib/core/CHIPPersistentStorageDelegate.h>
#include <lib/support/EnforceFormat.h>
#include <lib/support/logging/Constants.h>
#include <string.h>
Expand Down Expand Up @@ -80,8 +81,6 @@ class DefaultStorageKeyAllocator
static const char * OTAUpdateToken() { return "o/ut"; }

private:
static const size_t kKeyLengthMax = 32;

// The ENFORCE_FORMAT args are "off by one" because this is a class method,
// with an implicit "this" as first arg.
const char * ENFORCE_FORMAT(2, 3) Format(const char * format, ...)
Expand All @@ -93,7 +92,7 @@ class DefaultStorageKeyAllocator
return mKeyName;
}

char mKeyName[kKeyLengthMax + 1] = { 0 };
char mKeyName[PersistentStorageDelegate::kKeyLengthMax + 1] = { 0 };
};

} // namespace chip
85 changes: 70 additions & 15 deletions src/lib/support/TestPersistentStorageDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,66 +23,121 @@
#include <lib/core/CHIPCore.h>
#include <lib/core/CHIPPersistentStorageDelegate.h>
#include <lib/support/DLLUtil.h>
#include <lib/support/SafeInt.h>
#include <map>
#include <set>
#include <string>
#include <vector>

namespace chip {

/**
* Implementation of PersistentStorageDelegate suitable for unit tests,
* where persistence lasts for the object's lifetime and where all data is retained
* is memory.
*
* This version also has "poison keys" which, if accessed, yield an error. This can
* be used in unit tests to make sure a module making use of the PersistentStorageDelegate
* does not access some particular keys which should remain untouched by underlying
* logic.
*/
class TestPersistentStorageDelegate : public PersistentStorageDelegate
{
public:
TestPersistentStorageDelegate() {}

CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
{
if (mErrorKeys.find(std::string(key)) != mErrorKeys.end())
if ((buffer == nullptr) && (size != 0))
{
return CHIP_ERROR_INVALID_ARGUMENT;
}

// Making sure poison keys are not accessed
if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
{
return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
}

bool contains = mStorage.find(key) != mStorage.end();
VerifyOrReturnError(contains, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);

std::vector<uint8_t> & value = mStorage[key];
uint16_t value_size = static_cast<uint16_t>(value.size());
VerifyOrReturnError(value_size <= size, CHIP_ERROR_BUFFER_TOO_SMALL);

size = std::min(value_size, size);
memcpy(buffer, value.data(), size);
return CHIP_NO_ERROR;
size_t valueSize = value.size();
if (size < valueSize)
{
size = CanCastTo<uint16_t>(valueSize) ? static_cast<uint16_t>(valueSize) : 0;
return CHIP_ERROR_BUFFER_TOO_SMALL;
}
else
{
size = static_cast<uint16_t>(valueSize);
memcpy(buffer, value.data(), size);
return CHIP_NO_ERROR;
}
}

CHIP_ERROR SyncSetKeyValue(const char * key, const void * value, uint16_t size) override
{
if (mErrorKeys.find(std::string(key)) != mErrorKeys.end())
// Make sure poison keys are not accessed
if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
{
return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
}
const uint8_t * bytes = static_cast<const uint8_t *>(value);
mStorage[key] = std::vector<uint8_t>(bytes, bytes + size);
return CHIP_NO_ERROR;

// Handle empty values
if (value == nullptr)
{
if (size == 0)
{
mStorage[key] = std::vector<uint8_t>();
return CHIP_NO_ERROR;
}
else
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
}
// Handle non-empty values
else
{
const uint8_t * bytes = static_cast<const uint8_t *>(value);
mStorage[key] = std::vector<uint8_t>(bytes, bytes + size);
return CHIP_NO_ERROR;
}
}

CHIP_ERROR SyncDeleteKeyValue(const char * key) override
{
if (mErrorKeys.find(std::string(key)) != mErrorKeys.end())
// Make sure poison keys are not accessed
if (mPoisonKeys.find(std::string(key)) != mPoisonKeys.end())
{
return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
}

bool contains = mStorage.find(key) != mStorage.end();
VerifyOrReturnError(contains, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
mStorage.erase(key);
return CHIP_NO_ERROR;
}

void AddErrorKey(const std::string & key) { mErrorKeys.insert(key); }
/**
* @brief Adds a "poison key": a key that, if read/written, implies some bad
* behavior occurred.
*
* @param key - Poison key to add to the set.
*/
void AddPoisonKey(const std::string & key) { mPoisonKeys.insert(key); }

void ClearErrorKey() { mErrorKeys.clear(); }
/**
* @brief Clear all "poison keys"
*
*/
void ClearPoisonKeys() { mPoisonKeys.clear(); }

protected:
std::map<std::string, std::vector<uint8_t>> mStorage;
std::set<std::string> mErrorKeys;
std::set<std::string> mPoisonKeys;
};

} // namespace chip
1 change: 1 addition & 0 deletions src/lib/support/tests/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ chip_test_suite("tests") {
"TestSpan.cpp",
"TestStateMachine.cpp",
"TestStringBuilder.cpp",
"TestTestPersistentStorageDelegate.cpp",
"TestThreadOperationalDataset.cpp",
"TestTimeUtils.cpp",
"TestTlvToJson.cpp",
Expand Down
Loading

0 comments on commit 1f0307d

Please sign in to comment.