Skip to content

Commit

Permalink
Support raw ByteSpan values correctly in SafeAttributePersistenceProv…
Browse files Browse the repository at this point in the history 
…ider (project-chip#34306)

* Support raw ByteSpan values correctly in SafeattributePersistenceProvider

The DefaultAttributePersistenceProvider implementation of SafeReadValue() had
the undocumented behavior of enforcing that the value exactly filled the
provided buffer. This does not make sense for an API that purports to store
general ByteSpan values.

Instead move this validation into ReadScalarValue() in
SafeAttributePersistenceProvider, and separate InternalReadValue() into one
method that does the only the actual read, and a second one that performs the
additional validation for reading typed ember attributes (strings with length
prefixes etc). Use the former one from SafeReadValue().

Also fix possible out of bounds access when reading ember strings by ensuring
we have read the length byte(s) before attempting to interpret them.

* Take integral argument T by value

* Add test
  • Loading branch information
@ksperling-apple @j-ororke
ksperling-apple authored and j-ororke committed Jul 18, 2024
1 parent 077ca3c commit aa97f26
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 35 deletions.
27 changes: 16 additions & 11 deletions src/app/DefaultAttributePersistenceProvider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,34 +37,40 @@ CHIP_ERROR DefaultAttributePersistenceProvider::InternalWriteValue(const Storage
return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast<uint16_t>(aValue.size()));
}

CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType,
size_t aSize, MutableByteSpan & aValue)
CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue)
{
VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);

uint16_t size = static_cast<uint16_t>(min(aValue.size(), static_cast<size_t>(UINT16_MAX)));
ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size));
EmberAfAttributeType type = aType;
if (emberAfIsStringAttributeType(type))
aValue.reduce_size(size);
return CHIP_NO_ERROR;
}

CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType,
size_t aExpectedSize, MutableByteSpan & aValue)
{
ReturnErrorOnFailure(InternalReadValue(aKey, aValue));
size_t size = aValue.size();
if (emberAfIsStringAttributeType(aType))
{
// Ensure that we've read enough bytes that we are not ending up with
// un-initialized memory. Should have read length + 1 (for the length
// byte).
VerifyOrReturnError(size >= emberAfStringLength(aValue.data()) + 1, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(size >= 1 && size - 1 >= emberAfStringLength(aValue.data()), CHIP_ERROR_INCORRECT_STATE);
}
else if (emberAfIsLongStringAttributeType(type))
else if (emberAfIsLongStringAttributeType(aType))
{
// Ensure that we've read enough bytes that we are not ending up with
// un-initialized memory. Should have read length + 2 (for the length
// bytes).
VerifyOrReturnError(size >= emberAfLongStringLength(aValue.data()) + 2, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(size >= 2 && size - 2 >= emberAfLongStringLength(aValue.data()), CHIP_ERROR_INCORRECT_STATE);
}
else
{
// Ensure we got the expected number of bytes for all other types.
VerifyOrReturnError(size == aSize, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(size == aExpectedSize, CHIP_ERROR_INVALID_ARGUMENT);
}
aValue.reduce_size(size);
return CHIP_NO_ERROR;
}

Expand All @@ -90,8 +96,7 @@ CHIP_ERROR DefaultAttributePersistenceProvider::SafeWriteValue(const ConcreteAtt
CHIP_ERROR DefaultAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue)
{
return InternalReadValue(
DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), 0x20,
aValue.size(), aValue);
DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue);
}

namespace {
Expand Down
4 changes: 3 additions & 1 deletion src/app/DefaultAttributePersistenceProvider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider,

private:
CHIP_ERROR InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue);
CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aSize, MutableByteSpan & aValue);
CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue);
CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aExpectedSize,
MutableByteSpan & aValue);
};

} // namespace app
Expand Down
40 changes: 18 additions & 22 deletions src/app/SafeAttributePersistenceProvider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ class SafeAttributePersistenceProvider

// The following API provides helper functions to simplify the access of commonly used types.
// The API may not be complete.
// Currently implemented write and read types are: uint8_t, uint16_t, uint32_t, unit64_t and
// their nullable varieties, and bool.
// Currently implemented write and read types are: bool, uint8_t, uint16_t, uint32_t, unit64_t and
// their nullable varieties, as well as ByteSpans.

/**
* Write an attribute value of type intX, uintX or bool to non-volatile memory.
Expand All @@ -50,7 +50,7 @@ class SafeAttributePersistenceProvider
* @param [in] aValue the data to write.
*/
template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue)
CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T aValue)
{
uint8_t value[sizeof(T)];
auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T));
Expand All @@ -71,18 +71,16 @@ class SafeAttributePersistenceProvider
*
* @param [in] aPath the attribute path for the data being persisted.
* @param [in,out] aValue where to place the data.
*
* @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute
*/
template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, T & aValue)
{
uint8_t attrData[sizeof(T)];
MutableByteSpan tempVal(attrData);
auto err = SafeReadValue(aPath, tempVal);
if (err != CHIP_NO_ERROR)
{
return err;
}

ReturnErrorOnFailure(SafeReadValue(aPath, tempVal));
VerifyOrReturnError(tempVal.size() == sizeof(T), CHIP_ERROR_INCORRECT_STATE);
Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size());
r.RawReadLowLevelBeCareful(&aValue);
return r.StatusCode();
Expand All @@ -95,7 +93,7 @@ class SafeAttributePersistenceProvider
* @param [in] aValue the data to write.
*/
template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, const DataModel::Nullable<T> & aValue)
{
typename NumericAttributeTraits<T>::StorageType storageValue;
if (aValue.IsNull())
Expand All @@ -114,16 +112,14 @@ class SafeAttributePersistenceProvider
*
* @param [in] aPath the attribute path for the data being persisted.
* @param [in,out] aValue where to place the data.
*
* @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute
*/
template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
{
typename NumericAttributeTraits<T>::StorageType storageValue;
CHIP_ERROR err = ReadScalarValue(aPath, storageValue);
if (err != CHIP_NO_ERROR)
{
return err;
}
ReturnErrorOnFailure(ReadScalarValue(aPath, storageValue));

if (NumericAttributeTraits<T>::IsNullValue(storageValue))
{
Expand All @@ -137,25 +133,25 @@ class SafeAttributePersistenceProvider
return CHIP_NO_ERROR;
}

protected:
/**
* Write an attribute value from the attribute store (i.e. not a struct or
* list) to non-volatile memory.
*
* @param [in] aPath the attribute path for the data being written.
* @param [in] aValue the data to write. The value should be stored as-is.
* @param [in] aValue the data to write. The value will be stored as-is.
*/
virtual CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) = 0;

/**
* Read an attribute value from non-volatile memory.
* It can be assumed that this method will never be called upon to read
* an attribute of type string or long-string.
* Read an attribute value as a raw sequence of bytes from non-volatile memory.
*
* @param [in] aPath the attribute path for the data being persisted.
* @param [in,out] aValue where to place the data. The size of the buffer
* will be equal to `aValue.size()`. The callee is expected to adjust
* aValue's size to the actual number of bytes read.
* will be equal to `aValue.size()`. On success aValue.size()
* will be the actual number of bytes read.
*
* @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute
* @retval CHIP_ERROR_BUFFER_TOO_SMALL aValue.size() is too small to hold the value.
*/
virtual CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) = 0;
};
Expand Down
9 changes: 8 additions & 1 deletion src/app/tests/TestAttributePersistenceProvider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,13 @@ TEST_F(TestAttributePersistenceProvider, TestStorageAndRetrivalByteSpans)
MutableByteSpan valueReadBack(getArray);
err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack);
EXPECT_EQ(err, CHIP_NO_ERROR);
EXPECT_TRUE(std::equal(valueReadBack.begin(), valueReadBack.end(), value.begin(), value.end()));
EXPECT_TRUE(valueReadBack.data_equal(value));

uint8_t getArrayThatIsLongerThanNeeded[10];
MutableByteSpan valueReadBack2(getArrayThatIsLongerThanNeeded);
err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack2);
EXPECT_EQ(err, CHIP_NO_ERROR);
EXPECT_TRUE(valueReadBack2.data_equal(value));

// Finishing
persistenceProvider.Shutdown();
Expand Down Expand Up @@ -320,6 +326,7 @@ TEST_F(TestAttributePersistenceProvider, TestBufferTooSmallErrors)
err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBackByteSpan8);
EXPECT_EQ(err, CHIP_ERROR_BUFFER_TOO_SMALL);

// TODO: ReadScalarValue() does not take a buffer, so expecting CHIP_ERROR_BUFFER_TOO_SMALL is bad API
// Fail to get value as uint8_t
uint8_t valueReadBack8;
err = persistenceProvider.ReadScalarValue(TestConcretePath, valueReadBack8);
Expand Down

0 comments on commit aa97f26

Please sign in to comment.