Skip to content

Commit

Permalink
[app] Fix DeferredAttributePersister memory leak (#31075)
Browse files Browse the repository at this point in the history
* [app] Fix DeferredAttributePerister memory leak

ScopedMemoryBuffer's Release() method was used instead of
Free(). Add CHECK_RETURN_VALUE annotation to the Release()
method to prevent from making such a mistake in the future.

Signed-off-by: Damian Krolik <[email protected]>

* Code review

---------

Signed-off-by: Damian Krolik <[email protected]>
Damian-Nordic authored Dec 21, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent ca577f4 commit 3e8aeeb
Showing 3 changed files with 24 additions and 12 deletions.
2 changes: 1 addition & 1 deletion src/app/DeferredAttributePersistenceProvider.cpp
Original file line number Diff line number Diff line change
@@ -39,7 +39,7 @@ void DeferredAttribute::Flush(AttributePersistenceProvider & persister)
{
VerifyOrReturn(IsArmed());
persister.WriteValue(mPath, ByteSpan(mValue.Get(), mValue.AllocatedSize()));
mValue.Release();
mValue.Free();
}

CHIP_ERROR DeferredAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
4 changes: 2 additions & 2 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
Original file line number Diff line number Diff line change
@@ -1142,8 +1142,8 @@ - (void)readAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullable)attri
//
callback->AdoptReadClient(std::move(readClient));
callback.release();
attributePathParamsList.Release();
eventPathParamsList.Release();
IgnoreUnusedVariable(attributePathParamsList.Release());
IgnoreUnusedVariable(eventPathParamsList.Release());
return err;
});
std::move(*bridge).DispatchAction(self);
30 changes: 21 additions & 9 deletions src/lib/support/ScopedBuffer.h
Original file line number Diff line number Diff line change
@@ -25,6 +25,7 @@
#pragma once

#include <lib/support/CHIPMem.h>
#include <lib/support/CodeUtils.h>

#include <type_traits>
#include <utility>
@@ -84,10 +85,11 @@ class ScopedMemoryBufferBase
const void * Ptr() const { return mBuffer; }

/**
* Releases the undelying buffer. Buffer stops being managed and will not be
* auto-freed.
* Releases the underlying buffer.
*
* The buffer stops being managed and will not be auto-freed.
*/
void * Release()
CHECK_RETURN_VALUE void * Release()
{
void * buffer = mBuffer;
mBuffer = nullptr;
@@ -139,13 +141,18 @@ class ScopedMemoryBuffer : public Impl::ScopedMemoryBufferBase<MemoryManagement>

static_assert(std::is_trivially_destructible<T>::value, "Destructors won't get run");

inline T * Get() { return static_cast<T *>(Base::Ptr()); }
inline T & operator[](size_t index) { return Get()[index]; }
T * Get() { return static_cast<T *>(Base::Ptr()); }
T & operator[](size_t index) { return Get()[index]; }

inline const T * Get() const { return static_cast<const T *>(Base::Ptr()); }
inline const T & operator[](size_t index) const { return Get()[index]; }
const T * Get() const { return static_cast<const T *>(Base::Ptr()); }
const T & operator[](size_t index) const { return Get()[index]; }

inline T * Release() { return static_cast<T *>(Base::Release()); }
/**
* Releases the underlying buffer.
*
* The buffer stops being managed and will not be auto-freed.
*/
CHECK_RETURN_VALUE T * Release() { return static_cast<T *>(Base::Release()); }

ScopedMemoryBuffer & Calloc(size_t elementCount)
{
@@ -222,7 +229,12 @@ class ScopedMemoryBufferWithSize : public ScopedMemoryBuffer<T>
ScopedMemoryBuffer<T>::Free();
}

T * Release()
/**
* Releases the underlying buffer.
*
* The buffer stops being managed and will not be auto-freed.
*/
CHECK_RETURN_VALUE T * Release()
{
T * buffer = ScopedMemoryBuffer<T>::Release();
mCount = 0;

0 comments on commit 3e8aeeb

Please sign in to comment.