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

andy31415 · 2020-10-01T17:15:49Z

Problem

Pairing malloc and frees is easily error prone. In C++ we can use RAII.

Summary of Changes

Defines a smart pointer around Platform::MemoryAlloc and friends. Tested via Godbolt that this is a 0-cost abstraction.

used this new class in a couple of places. Specifically for linux storage, I also refactored the layout of some functions to avoid the large if nesting (was even missing an out of memory check which would have made it larger) at the expese of not having a single return (I believe we overly try for a single return and this hurts readability).

Fixes #2896

Platform::Memory.

andy31415 · 2020-10-01T17:18:13Z

For reference, in a malloc/free wrapper for scoped memory:

size_t test() {
    ScopedMemoryBuffer b;

    b.Alloc(256);
    strcpy(b.Ptr<char>(), "this is a test");

    return strlen(b.Ptr<char>());
}

results in this on gcc:

.LC0:
        .ascii  "this is a test\000"
test():
        push    {r4, r5, r6, lr}
        mov     r0, #256
        bl      malloc
        mov     r4, r0
        ldr     r1, .L3
        bl      strcpy
        mov     r0, r4
        bl      strlen
        mov     r5, r0
        mov     r0, r4
        bl      free
        mov     r0, r5
        pop     {r4, r5, r6, lr}
        bx      lr
.L3:
        .word   .LC0

mspang · 2020-10-01T18:02:16Z

Problem

Pairing malloc and frees is easily error prone. In C++ we can use RAII.

Summary of Changes

Defines a smart pointer around Platform::MemoryAlloc and friends. Tested via Godbolt that this is a 0-cost abstraction.

Did you test single vs multiple returns on godbolt? I think that's more likely where costs will be incurred.

used this new class in a couple of places. Specifically for linux storage, I also refactored the layout of some functions to avoid the large if nesting (was even missing an out of memory check which would have made it larger) at the expese of not having a single return (I believe we overly try for a single return and this hurts readability).

Fixes #2896

src/lib/support/ScopedBuffer.h

bzbarsky-apple · 2020-10-01T17:57:37Z

src/lib/support/ScopedBuffer.h

+    operator bool() const { return mBuffer != nullptr; }
+
+    /** Take over managing memory from the specified buffer */
+    ScopedMemoryBufferBase & Aquire(void * buffer)


"Acquire". Maybe "Adopt" is a better name?

how does the instance know where to call to free the pointer? the buffer in question has to be understood by the Impl's static MemoryFree() function?

I'd really like to see a callback (or a closure) passed in here.

I believe this is a crutch for existing code if memory is already managed 'the old way'.
We can assume that any porting/new code can start to use this code from the start and then drop this method.

bzbarsky-apple · 2020-10-01T17:59:33Z

src/lib/support/ScopedBuffer.h

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


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....

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

src/lib/support/ScopedBuffer.h

src/lib/support/tests/TestScopedBuffer.cpp

bzbarsky-apple · 2020-10-01T18:07:32Z

src/include/platform/internal/GenericConfigurationManagerImpl.ipp

@@ -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;


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

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

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.

bzbarsky-apple · 2020-10-01T18:08:12Z

src/lib/support/ScopedBuffer.h

+    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); }


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

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.

bzbarsky-apple · 2020-10-01T18:08:39Z

src/lib/support/tests/TestScopedBuffer.cpp

+
+#include <nlunit-test.h>
+
+using namespace chip;


This should obviate the need for the chip:: in various places in this file.

Dropped the line. we don't have too many chip:: remaining so this should be fine. Somehow Platform cannot be without chip:: prefix so the worst offender cannot be removed.

Actually rechecked and in this context I could remove chip:: and keep just Platform. Still unsure if worth it.

bzbarsky-apple · 2020-10-01T18:09:58Z

src/platform/Linux/CHIPLinuxStorageIni.cpp

-    CHIP_ERROR retval  = CHIP_NO_ERROR;
-    char * encodedData = nullptr;
+    CHIP_ERROR retval = CHIP_NO_ERROR;
+    chip::Platform::ScopedMemoryBuffer encodedData;


This is inside namespace chip?

You would think so, but Inet and storage have different ones :(

../../src/platform/Linux/CHIPLinuxStorageIni.cpp:222:79: error: reference to ‘Platform’ is ambiguous 222 | CHIP_ERROR ChipLinuxStorageIni::GetBinaryBlobDataAndLengths(const char * key, Platform::ScopedMemoryBuffer & encodedData, | ^~~~~~~~ In file included from ../../src/lib/core/CHIPCore.h:39, from ../../src/include/platform/CHIPDeviceLayer.h:25, from ../../src/include/platform/internal/CHIPDeviceLayerInternal.h:23, from ../../src/platform/Linux/CHIPLinuxStorageIni.cpp:31: ../../src/inet/InetLayer.h:111:11: note: candidates are: ‘namespace chip::Inet::Platform { }’ 111 | namespace Platform { | ^~~~~~~~ In file included from ../../src/platform/Linux/CHIPLinuxStorageIni.h:30, from ../../src/platform/Linux/CHIPLinuxStorageIni.cpp:30: ../../src/include/platform/PersistedStorage.h:34:11: note: ‘namespace chip::Platform { }’ 34 | namespace Platform {

I don't want to refactor too much, so keeping the super specific bit.

Yeah, I didn't know we had multiple Platform namespaces. Just ignore all the namespace comments.

bzbarsky-apple · 2020-10-01T18:17:48Z

src/lib/support/ScopedBuffer.h

+    void * Ptr() { return mBuffer; }
+    const void * Ptr() const { return mBuffer; }
+
+    template <typename T>


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

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.

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

If we have static knowledge about alignment of the things our allocator returns maybe static_assert that Type does not have stronger alignment requirements.

Add conversion operators returning Type* and const Type*.

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?

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.

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.

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?

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.

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.

bzbarsky-apple · 2020-10-01T18:18:27Z

Also, this is excellent. :)

rwalker-apple · 2020-10-01T19:19:08Z

src/include/platform/internal/GenericConfigurationManagerImpl.ipp

        }

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


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

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.

clearsecretdata() takes a uint8_t*

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?

rwalker-apple · 2020-10-01T19:27:59Z

src/lib/support/ScopedBuffer.h

+    ScopedMemoryBufferBase & Alloc(size_t size)
+    {
+        Free();
+        mBuffer = Impl::MemoryAlloc(size);


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

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.

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) {} ... };

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.

mspang · 2020-10-02T19:40:26Z

src/lib/support/ScopedBuffer.h

+    void * Ptr() { return mBuffer; }
+    const void * Ptr() const { return mBuffer; }
+
+    template <typename T>


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.

andy31415 · 2020-10-02T20:30:36Z

Oh ... just had a refactor to address Michael/Boris coments regarding typesafety.

Will submit them as a separate PR.

Add a 'Scoped Buffer' implementation as smart-pointer against

b0f35f6

Platform::Memory.

boring-cyborg bot added lib linux platform labels Oct 1, 2020

restyled-io bot mentioned this pull request Oct 1, 2020

Restyle Add a 'Scoped Buffer' implementation as smart-pointer #2988

Closed

add newline at the end of ScopedBuffer header

d48c1aa

pullapprove bot requested review from BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple and saurabhst October 1, 2020 17:33

andy31415 mentioned this pull request Oct 1, 2020

Figure out a way to express PacketBuffer ownership in the type system #2707

Closed

bzbarsky-apple reviewed Oct 1, 2020

View reviewed changes

rwalker-apple reviewed Oct 1, 2020

View reviewed changes

rwalker-apple approved these changes Oct 1, 2020

View reviewed changes

andy31415 added 3 commits October 2, 2020 10:01

Updated based on code reviews

26ea54c

Added comment that void* on the scoped memory ptr is on purpose

49b963b

Add ptr call to clear secret data

48eba90

boring-cyborg bot added the nrf5 label Oct 2, 2020

BroderickCarlin approved these changes Oct 2, 2020

View reviewed changes

Merge branch 'master' into 01_scoped_buffer

d328dcb

mspang approved these changes Oct 2, 2020

View reviewed changes

rwalker-apple merged commit f43f552 into project-chip:master Oct 2, 2020

rwalker-apple mentioned this pull request Oct 2, 2020

Template parameter in ScopedBuffer::Ptr is no more than an obscured cast, consider removal #3025

Closed

bzbarsky-apple mentioned this pull request Oct 5, 2020

Fix scoped buffer test to init and shut down Platform::Memory. #3051

Closed

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

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

Conversation

andy31415 commented Oct 1, 2020

Problem

Summary of Changes

andy31415 commented Oct 1, 2020

mspang commented Oct 1, 2020

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky-apple commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Damian-Nordic Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy31415 commented Oct 2, 2020

Damian-Nordic Oct 2, 2020 •

edited

Loading