Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a 'Scoped Buffer' implementation as smart-pointer #2987

Merged
merged 6 commits into from
Oct 2, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions src/include/platform/internal/GenericConfigurationManagerImpl.ipp
Original file line number Diff line number Diff line change
@@ -32,6 +32,7 @@
#include <support/Base64.h>
#include <support/CHIPMem.h>
#include <support/CodeUtils.h>
#include <support/ScopedBuffer.h>

#if CHIP_DEVICE_CONFIG_ENABLE_THREAD
#include <platform/ThreadStackManager.h>
@@ -867,7 +868,7 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ComputeProvisioningHash(
using HashAlgo = chip::Crypto::Hash_SHA256_stream;

HashAlgo hash;
uint8_t * dataBuf = NULL;
chip::Platform::ScopedMemoryBuffer dataBuf;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inside namespace chip, right? Why the prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazingly enough compiler complained about Platform being ambigous, so I used it as such.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically at least chip::Platform and chip::System::Platform exist and for code that has 'using' on both chip and system, we get conflicts.

We should not get conflicts if headers had a rule of 'no using inside headers' but the ported code does not enforce that so we are stuck until we clean up code. Would prefer that cleanup to be independent of this PR.

size_t dataBufSize;
constexpr uint16_t kLenFieldLen = 4; // 4 hex characters

@@ -920,11 +921,10 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ComputeProvisioningHash(
// Create a temporary buffer to hold the certificate. (This will also be used for
// the private key).
dataBufSize = certLen;
dataBuf = (uint8_t *) chip::Platform::MemoryAlloc(dataBufSize);
VerifyOrExit(dataBuf != NULL, err = CHIP_ERROR_NO_MEMORY);
VerifyOrExit(dataBuf.Alloc(dataBufSize), err = CHIP_ERROR_NO_MEMORY);

// Read the certificate.
err = Impl()->_GetManufacturerDeviceCertificate(dataBuf, certLen, certLen);
err = Impl()->_GetManufacturerDeviceCertificate(dataBuf.Ptr<uint8_t>(), certLen, certLen);
SuccessOrExit(err);
}

@@ -941,15 +941,12 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ComputeProvisioningHash(
// (This will also be used for the private key).
if (certsLen > dataBufSize)
{
chip::Platform::MemoryFree(dataBuf);

dataBufSize = certsLen;
dataBuf = (uint8_t *) chip::Platform::MemoryAlloc(dataBufSize);
VerifyOrExit(dataBuf != NULL, err = CHIP_ERROR_NO_MEMORY);
VerifyOrExit(dataBuf.Alloc(dataBufSize), err = CHIP_ERROR_NO_MEMORY);
}

// Read the device intermediate CA certificates.
err = Impl()->_GetManufacturerDeviceIntermediateCACerts(dataBuf, certsLen, certsLen);
err = Impl()->_GetManufacturerDeviceIntermediateCACerts(dataBuf.Ptr<uint8_t>(), certsLen, certsLen);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this <uint8_t> templating necessary here, but not below for ClearSecretData?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried verbatim copy. Data was uint8_t by default hence conversions everywhere, but clear secret data takes void* I think. I did not check the rest.

Speaking of which: clearsecretdata is called on the last free, however we have an internal alloc that does NOT clear it. Unsure if intentional or not, so I kept 'as is' but I do assume a bug where using this class would have made easier to spot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clearsecretdata() takes a uint8_t*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... new theory (verified though): when I do 'run functional and unit tests' in vscode, this file does not get called at all, so it does not notice that the clearsecretdata call is bad.

I updated it blindly and will rely on CI to check things. This is very odd... is this file even used?

SuccessOrExit(err);
}

@@ -964,7 +961,7 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ComputeProvisioningHash(
// Read the private key. (Note that we presume the buffer allocated to hold the certificate
// is big enough to hold the private key. _GetDevicePrivateKey() will return an error in the
// unlikely event that this is not the case.)
err = Impl()->_GetManufacturerDevicePrivateKey(dataBuf, dataBufSize, keyLen);
err = Impl()->_GetManufacturerDevicePrivateKey(dataBuf.Ptr<uint8_t>(), dataBufSize, keyLen);
SuccessOrExit(err);
}

@@ -991,10 +988,9 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ComputeProvisioningHash(
hash.Finish(hashBuf);

exit:
if (dataBuf != NULL)
if (dataBuf)
{
chip::Crypto::ClearSecretData(dataBuf, dataBufSize);
chip::Platform::MemoryFree(dataBuf);
chip::Crypto::ClearSecretData(dataBuf.Ptr(), dataBufSize);
}
#endif // CHIP_DEVICE_CONFIG_LOG_PROVISIONING_HASH

150 changes: 150 additions & 0 deletions src/lib/support/ScopedBuffer.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* @file
* Defines scoped auto-free buffers for CHIP.
*
*/

#ifndef CHIP_SCOPED_BUFFER_H_
#define CHIP_SCOPED_BUFFER_H_

#include <lib/support/CHIPMem.h>

namespace chip {
namespace Platform {

namespace Impl {

/**
* Represents a memory buffer that is auto-freed in the destructor.
*
* This class uses void* underneath on purpose (rather than a unique_ptr like
* 'Type') and uses templated type on Ptr(). This is to avoid template explosion
* when the buffers are used for different types - only one implementation of
* the class will be stored in flash.
*/
template <class Impl>
class ScopedMemoryBufferBase
{
public:
ScopedMemoryBufferBase() {}
ScopedMemoryBufferBase(const ScopedMemoryBufferBase &) = delete;
ScopedMemoryBufferBase & operator=(const ScopedMemoryBufferBase & other) = delete;

~ScopedMemoryBufferBase() { Free(); }

void * Ptr() { return mBuffer; }
const void * Ptr() const { return mBuffer; }

template <typename T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether it makes sense to make the following changes:

  1. Add a template parameter for the type of the things in the buffer (to either the base class or the underlying class). Let's call that type Type.
  2. static_assert that Type is a POD type, to avoid issues with people putting things with destructors (which will not get run) in the buffer
  3. If we have static knowledge about alignment of the things our allocator returns maybe static_assert that Type does not have stronger alignment requirements.
  4. Add conversion operators returning Type* and const Type*.
  5. Maybe add an operator[] returning Type& (and a const equivalent).

This would still allow effectively doing reinterpret_cast via the Ptr method if we want to allow that, but would also allow "normal" use to specify the type up front and not have to mention it at every place where the buffer is actually being used. The various places in the changeset that use someBuffer.Ptr<sometype>() could just use someBuffer.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that however my intent is to avoid template explosion (this was the main objection I heard about using templates in embedded). I do not want duplicate implementations inside flash for uint8_t, char, etc.

In this approach, we have only the Get duplicated, but that one is a trivial function and inlined, so it should go away and we get 0 cost abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may refactor it later... maybe subclassing works without exploding things (just make cast operators easy to use). I would defer it for the future though. At first I thought that this Memory alloc/free is used all over in chip, but in reality we have very limited places where we actually use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe subclassing works without exploding things

Right. Codesize explosion is a good reason to not template the base class. Templating the derived class, and making sure that the templated thing only has inline methods, would avoid the codesize problems, though.

I'm probably ok with this change as a followup, especially since it would involve codesize measurements, but I do think we should make it, not just for ease of use but also because it enables us to add safety asserts (like the is_pod assertion I suggested). Are you willing to take the time to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it, please add a PR and assign it to me.
I am unsure timing wise, however it seems small enough of a change that I can do it.
Also thinking we probably want a implementation for 'secure memory' to call a clear on every free. Our existing code seems somewhat buggy in that regard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Boris, the current implementation strikes me as type-unsafe and we should be able to fix it without increasing code size.

I would suggest removing the template parameter from Ptr<> in this initial PR, that's really just hiding a typecast under a less obvious syntax now. Potentially unsafe casts are the last thing we want to hide in a benign-looking accessor function.

inline T * Ptr()
{
return static_cast<T *>(mBuffer);
}

template <typename T>
inline const T * Ptr() const
{
return static_cast<T *>(mBuffer);
}

ScopedMemoryBufferBase & Alloc(size_t size)
{
Free();
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
mBuffer = Impl::MemoryAlloc(size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any instance data for the Impl? what if we want to allocate from pools, or allocate and free across linking boundaries?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impl was just just because I needed to test things. In reality these are supposed to be Platform::Memory* methods which are not inside a class.

Static polyphormisms seems awkward, but at least has no cost and is testable.

Copy link
Contributor

@Damian-Nordic Damian-Nordic Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if that's useful at all, but you could probably support stateful allocators, by reversing the class hierarchy and following STL convention:

struct DefaultAllocator
{
    static void MemoryFree(void * p) { chip::Platform::MemoryFree(p); }
    static void * MemoryAlloc(size_t size) { return chip::Platform::MemoryAlloc(size); }
    static void * MemoryAlloc(size_t size, bool longTerm) { return chip::Platform::MemoryAlloc(size, longTerm); }
    static void * MemoryCalloc(size_t num, size_t size) { return chip::Platform::MemoryCalloc(num, size); }
};

template <class Allocator=DefaultAllocator>
class ScopedMemoryBuffer : Allocator
{
public:
    explicit ScopedMemoryBufferBase(const Allocator& a) : Allocator(a) {}
...
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the suggestion, however I am unsure of the effect of those on template code explosion. I believe we do not use std pointers because exceptions and because potential code bloat.

I would be happy to review this separately, however as a first pass I would prefer code that I was able to double-check with godbolt.

return *this;
}

ScopedMemoryBufferBase & LongTermAlloc(size_t size)
{
Free();
mBuffer = Impl::MemoryAlloc(size, true /* isLongTermAlloc */);
return *this;
}

template <typename T>
ScopedMemoryBufferBase & Calloc(size_t elementCount)
{
Free();
mBuffer = Impl::MemoryCalloc(elementCount, sizeof(T));
return *this;
}

/** Check if a buffer is valid */
explicit operator bool() const { return mBuffer != nullptr; }
bool operator!() const { return mBuffer == nullptr; }

/**
* Releases the undelying buffer. Buffer stops being managed and will not be
* auto-freed.
*/
void * Release()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to call this "Release", or "Forget"? I guess "Release" more closely matches the unique_ptr API, but then we should use "Reset" instead of "Acquire", maybe....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped aquire and thinking to keep release for matching with unique_ptr. This class is basically uniqe_ptr but without exception handling.

{
void * buffer = mBuffer;
mBuffer = nullptr;
return buffer;
}

template <typename T>
inline T * Release()
{
void * buffer = mBuffer;
mBuffer = nullptr;
return static_cast<T *>(buffer);
}

/** Release memory used */
void Free()
{
if (mBuffer == nullptr)
{
return;
}
Impl::MemoryFree(mBuffer);
mBuffer = nullptr;
}

private:
void * mBuffer = nullptr;
};

} // namespace Impl

/**
* Represents a memory buffer allocated using chip::Platform::Memory*Alloc
* methods.
*
* Use for RAII to auto-free after use.
*/
class ScopedMemoryBuffer : public Impl::ScopedMemoryBufferBase<ScopedMemoryBuffer>
{
friend class Impl::ScopedMemoryBufferBase<ScopedMemoryBuffer>;

protected:
static void MemoryFree(void * p) { chip::Platform::MemoryFree(p); }
static void * MemoryAlloc(size_t size) { return chip::Platform::MemoryAlloc(size); }
static void * MemoryAlloc(size_t size, bool longTerm) { return chip::Platform::MemoryAlloc(size, longTerm); }
static void * MemoryCalloc(size_t num, size_t size) { return chip::Platform::MemoryCalloc(num, size); }
Comment on lines +141 to +144
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this stuff is inside namespace chip, so why the prefixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we have some include ambiguity. I did not try to fix it but rather just be explicit. It looks slightly annoying, but not unreadable.

};

} // namespace Platform
} // namespace chip

#endif // CHIP_SCOPED_BUFFER_H_
2 changes: 2 additions & 0 deletions src/lib/support/tests/BUILD.gn
Original file line number Diff line number Diff line change
@@ -29,6 +29,7 @@ chip_test_suite("tests") {
"TestPersistedCounter.cpp",
"TestPersistedStorageImplementation.cpp",
"TestPersistedStorageImplementation.h",
"TestScopedBuffer.cpp",
"TestSupport.h",
"TestTimeUtils.cpp",
]
@@ -47,5 +48,6 @@ chip_test_suite("tests") {
"TestCHIPMem",
"TestCHIPCounter",
"TestPersistedCounter",
"TestScopedBuffer",
]
}
147 changes: 147 additions & 0 deletions src/lib/support/tests/TestScopedBuffer.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "TestSupport.h"

#include <support/ScopedBuffer.h>
#include <support/TestUtils.h>

#include <nlunit-test.h>

namespace {

class TestCounterScopedBuffer : public chip::Platform::Impl::ScopedMemoryBufferBase<TestCounterScopedBuffer>
{
public:
static int Counter() { return mAllocCount; }

static void MemoryFree(void * p)
{
mAllocCount--;
chip::Platform::MemoryFree(p);
}
static void * MemoryAlloc(size_t size)
{
mAllocCount++;
return chip::Platform::MemoryAlloc(size);
}
static void * MemoryAlloc(size_t size, bool longTerm)
{

mAllocCount++;
return chip::Platform::MemoryAlloc(size, longTerm);
}
static void * MemoryCalloc(size_t num, size_t size)
{

mAllocCount++;
return chip::Platform::MemoryCalloc(num, size);
}

private:
static int mAllocCount;
};
int TestCounterScopedBuffer::mAllocCount = 0;

void TestAutoFree(nlTestSuite * inSuite, void * inContext)
{
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);

{
TestCounterScopedBuffer buffer;

NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);
NL_TEST_ASSERT(inSuite, buffer.Alloc(128));
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 1);
}
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);
}

void TestFreeDuringAllocs(nlTestSuite * inSuite, void * inContext)
{
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);

{
TestCounterScopedBuffer buffer;

NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);
NL_TEST_ASSERT(inSuite, buffer.Alloc(128));
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 1);
NL_TEST_ASSERT(inSuite, buffer.Alloc(64));
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 1);
NL_TEST_ASSERT(inSuite, buffer.LongTermAlloc(256));
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 1);
NL_TEST_ASSERT(inSuite, buffer.Calloc<uint64_t>(10));
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 1);
}
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);
}

void TestRelease(nlTestSuite * inSuite, void * inContext)
{
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);
void * ptr = nullptr;

{
TestCounterScopedBuffer buffer;

NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);
NL_TEST_ASSERT(inSuite, buffer.Alloc(128));
NL_TEST_ASSERT(inSuite, buffer.Ptr() != nullptr);

ptr = buffer.Release();
NL_TEST_ASSERT(inSuite, ptr != nullptr);
NL_TEST_ASSERT(inSuite, buffer.Ptr() == nullptr);
}

NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 1);

{
TestCounterScopedBuffer buffer;
NL_TEST_ASSERT(inSuite, buffer.Alloc(128));
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 2);
TestCounterScopedBuffer::MemoryFree(ptr);
NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 1);
}

NL_TEST_ASSERT(inSuite, TestCounterScopedBuffer::Counter() == 0);
}

} // namespace

#define NL_TEST_DEF_FN(fn) NL_TEST_DEF("Test " #fn, fn)
/**
* Test Suite. It lists all the test functions.
*/
static const nlTest sTests[] = {
NL_TEST_DEF_FN(TestAutoFree), //
NL_TEST_DEF_FN(TestFreeDuringAllocs), //
NL_TEST_DEF_FN(TestRelease), //
NL_TEST_SENTINEL() //
};

int TestScopedBuffer(void)
{
nlTestSuite theSuite = { "CHIP ScopedBuffer tests", &sTests[0], nullptr, nullptr };

// Run test suit againt one context.
nlTestRunner(&theSuite, nullptr);
return nlTestRunnerStats(&theSuite);
}

CHIP_REGISTER_TEST_SUITE(TestBufBound)
Loading