Redesign allocator by al1img · Pull Request #636 · aosedge/aos_core_lib_cpp

al1img · 2026-07-31T14:38:10Z

No description provided.

MykolaSuperman · 2026-08-02T09:05:34Z

+private:
+    virtual void Dispose() = 0;
+
+    size_t mRefCount = 1;


[P1] SharedPtr is no longer thread-safe

The previous implementation serialized TakeAllocation and GiveAllocation using the allocator mutex. SharedControlBlock now increments and decrements the plain size_t mRefCount without synchronization. Copying and destroying SharedPtr instances from different threads is a data race and can lose reference-count updates, causing premature disposal, double disposal, or use-after-free. Please make the reference count atomic with appropriate memory ordering, or protect Take and Give with a mutex.

MykolaSuperman · 2026-08-02T11:17:14Z

-    auto stop  = MakeShared<StaticArray<InstanceIdent, cMaxNumInstances>>(&mAllocator, stopInstances);
-    auto start = MakeShared<InstanceInfoArray>(&mAllocator, startInstances);
+    auto stop = MakeShared<StaticArray<InstanceIdent, cMaxNumInstances>>(mAllocator, stopInstances);
+    if (!stop) {


[P1] Finish the launch on allocation failure

StartLaunch sets mLaunchInProgress before these allocations. If either MakeShared call fails, the return bypasses FinishLaunch, so subsequent updates return eWrongState and Stop can wait forever for mLaunchInProgress to clear. Please call FinishLaunch on both OOM paths, use a scope guard, or allocate before marking the launch as in progress.

Fixed, see: "tools: make SharedPtr control block ref count thread safe" commit. Currently, I use mutex to protect ref counter.

MykolaSuperman · 2026-08-02T11:17:19Z


-    auto session = MakeShared<SessionContext>(&mAllocator, handle, mFunctionList);
+    auto session = MakeShared<SessionContext>(mAllocator, handle, mFunctionList);
+    if (!session) {


[P2] Close the PKCS#11 session when allocation fails

C_OpenSession has already created a session handle at this point. If MakeShared fails, the function returns without calling C_CloseSession, and the SessionContext destructor that normally closes the handle is never created. This leaks a token session on every allocation failure and can eventually exhaust the token session limit. Please close handle before returning eNoMemory.

Fixed, see: "common: pkcs11: close session on SessionContext alloc failure" commit.

mlohvynenko · 2026-08-03T08:03:42Z

    Error Encrypt(const crypto::RSAPublicKey& pubKey, const Array<uint8_t>& msg, Array<uint8_t>& cipher) override;

 private:
+    HeapAllocator         mAllocator;


Why don't you pass AllocatorItf in Init for openssl and mbeedtls factories?

mykola-kobets-epam

Reviewed-by: Mykola Kobets <mykola_kobets@epam.com>

mykola-kobets-epam · 2026-08-03T09:23:58Z

    case UpdateItemTypeEnum::eService:
        newInstance = MakeShared<ServiceInstance>(
-            &mAllocator, info, mUIDPool, mGIDPool, *mStorage, mStorageState, mImageInfoProvider, mInstanceAllocator);
+            mAllocator, *mAllocator, info, mUIDPool, mGIDPool, *mStorage, mStorageState, mImageInfoProvider);


the same allocator provided twice in the param list

It is not the same allocator provided twice: first parameter is is MakeShared first param - allocator to allocate ServiceInstance. Second allocator is passed to ServiceInstance constructor as parameter.

mykola-kobets-epam · 2026-08-03T09:26:18Z

    case UpdateItemTypeEnum::eComponent:
-        newInstance
-            = MakeShared<ComponentInstance>(&mAllocator, info, *mStorage, mImageInfoProvider, mInstanceAllocator);
+        newInstance = MakeShared<ComponentInstance>(mAllocator, *mAllocator, info, *mStorage, mImageInfoProvider);


the same allocator provided twice in the param list

mlohvynenko

Reviewed-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>

MykolaSuperman

Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>

MakeUnique/MakeShared placement-constructed objects via the Allocator-based operator new, which only guarded a failed allocation with assert(). In release builds (NDEBUG) that assert is compiled out, so an exhausted allocator returned nullptr and the constructor still ran at a null address (UB). Both factories now call Allocator::Allocate() explicitly, check the result, and return an empty pointer instead of constructing when the allocation fails. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

MakeUnique/MakeShared already returned an empty (falsy) pointer on allocator exhaustion, but most call sites across the codebase never checked the result before dereferencing it, so an out-of-memory condition would still crash on a null-pointer dereference instead of being reported as an error. Add a check after every such call site, propagating eNoMemory using whichever convention the enclosing function already uses (Error, RetWithError<X>, bool, or void with a log message). Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Rework the memory allocation abstraction in memory.hpp: - Rename Allocator to AllocatorItf, keeping only Allocate/Free and a virtual destructor. This decouples callers from any particular allocation strategy. - Remove StaticAllocator, BufferAllocator and the custom placement new/delete operator overloads (allocator.hpp is deleted). Sizing a static arena correctly, especially for multithreaded usage, was error prone and required extra bookkeeping. - Add HeapAllocator (malloc/free backed) for Linux and test usage. Safety-critical targets can provide their own AllocatorItf implementation. - Rework SharedPtr to use intrusive control blocks (SharedControlBlock/SharedObjectControlBlock/SharedAdoptControlBlock) instead of allocator-external ref-counting, so it works uniformly over any AllocatorItf implementation. - MakeUnique/MakeShared now check the allocation result before constructing the object, returning a null pointer on failure instead of relying on assert(), which is stripped in release builds. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Convert common/ classes that previously owned a private static allocator to receive an AllocatorItf reference/pointer instead, following the two-phase construct-then-Init() pattern used across the codebase (allocator as the first Init()/constructor parameter): - crypto: CertLoader, CryptoHelper, mbedtls/openssl CryptoProviderItf implementations, pkcs11::Utils, PKCS11RSAPrivateKey. - pkcs11: LibraryContext, PKCS11Manager. - monitoring: Average, Monitoring. - spaceallocator: SpaceAllocator; also renamed its own "space" allocator members (OutdatedItem::mSpaceAllocator, the nested Space class's mSpaceAllocator) to avoid confusion with the new memory AllocatorItf member. - fs: CalculateSize takes an AllocatorItf parameter (first), and FileInfoProvider forwards it internally; dropped the shared static allocator and its guarding mutex, since callers now own their allocator's thread-safety. Multiple per-class named allocators are consolidated into a single AllocatorItf pointer where heap allocation removes the need for separate statically-sized pools. Unit tests construct a HeapAllocator and pass it in, always declared before any member that may allocate from it, since C++ destroys members in reverse declaration order. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Convert cm/ classes that previously owned a private static allocator to receive an AllocatorItf reference/pointer instead, following the two-phase construct-then-Init() pattern used across the codebase (allocator as the first Init() parameter, or constructor parameter for Instance/ComponentInstance/ServiceInstance which have no Init()): - alerts, imagemanager, nodeinfoprovider, storagestate, unitconfig, updatemanager (desiredstatushandler, unitstatushandler, updatemanager). - launcher: Launcher composition root and all of its composed sub-components (InstanceManager, ImageInfoProvider, StorageState, NodeManager, Node, Balancer, RunRequestsLoader), forwarding the same allocator instance through the whole ownership chain. Multiple per-class named allocators are consolidated into a single AllocatorItf pointer where heap allocation removes the need for separate statically-sized pools. Unit tests construct a HeapAllocator and pass it in, always declared before any member that may allocate from it, since C++ destroys members in reverse declaration order. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Convert iam/ classes that previously owned a private static allocator to receive an AllocatorItf reference/pointer instead, following the two-phase construct-then-Init() pattern used across the codebase (allocator as the first Init() parameter): - nodemanager: NodeManager. - certhandler: CertModule, PKCS11Module (consolidating its separate temp-object and local-cache allocators into a single AllocatorItf pointer). CertHandler has no Init(), so the allocator is passed through its constructor instead. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Convert sm/ classes that previously owned a private static allocator to receive an AllocatorItf reference/pointer instead, following the two-phase construct-then-Init() pattern used across the codebase (allocator as the first Init() parameter): - imagemanager: ImageManager. - launcher: Launcher. - networkmanager: NetworkManager (consolidating its separate network-info and resolv-hosts allocators into a single AllocatorItf pointer). - nodeconfig: NodeConfig. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

The map is keyed by instance ID only, so its capacity should be cMaxNumInstances rather than cMaxNumInstances * cMaxNumOwners, which oversized the allocation. Realign member declarations accordingly. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Remove unmatched suppression: templateRecursion from array. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

UpdateInstances() left mLaunchInProgress stuck true if StartLaunch() succeeded but the subsequent stop/start array allocation failed, since the early return skipped FinishLaunch(). Guard it with a DeferRelease that calls FinishLaunch() whenever the function exits with a non-none error, leaving the async thread responsible for FinishLaunch() on the success path. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

cppcheck flags mAllocator as an unused struct member since it is only ever referenced implicitly (as the source allocator passed to other members) rather than accessed directly. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

The AllocatorItf redesign dropped the mutex that previously guarded the shared pointer allocation's reference count inside the old Allocator class, leaving SharedControlBlock::Take/Give to increment and decrement a plain size_t with no synchronization. Concurrent copies/resets of a SharedPtr from multiple threads could therefore race on the ref count. Add a Mutex to SharedControlBlock guarding Take/Give, restoring the previous thread safety guarantee. Give() releases the lock before calling Dispose(), since disposal destroys the control block (and its mutex) itself. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

LibraryContext::PKCS11OpenSession opened a PKCS11 session but did not close it if allocating the SessionContext wrapper failed, leaking the underlying session handle. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

sonarqubecloud · 2026-08-04T10:40:42Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

al1img force-pushed the redesign_allocator branch from 998c34a to 28879db Compare July 31, 2026 15:19

MykolaSuperman reviewed Aug 2, 2026

View reviewed changes

mlohvynenko reviewed Aug 3, 2026

View reviewed changes

mykola-kobets-epam approved these changes Aug 3, 2026

View reviewed changes

mykola-kobets-epam reviewed Aug 3, 2026

View reviewed changes

al1img force-pushed the redesign_allocator branch 4 times, most recently from 6c08aa6 to dcb0ef2 Compare August 3, 2026 11:44

mlohvynenko approved these changes Aug 3, 2026

View reviewed changes

MykolaSuperman approved these changes Aug 3, 2026

View reviewed changes

al1img added 13 commits August 4, 2026 13:21

common: tools: remove unmatched suppression: templateRecursion

8073279

Remove unmatched suppression: templateRecursion from array. Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

al1img force-pushed the redesign_allocator branch from dcb0ef2 to 485e1b2 Compare August 4, 2026 10:29

al1img merged commit 4d11643 into aosedge:develop Aug 4, 2026
4 of 5 checks passed

Uh oh!

Conversation

al1img commented Jul 31, 2026

Uh oh!

MykolaSuperman Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

al1img Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-kobets-epam left a comment

Choose a reason for hiding this comment

Uh oh!

mykola-kobets-epam Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlohvynenko left a comment

Choose a reason for hiding this comment

Uh oh!

MykolaSuperman left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Aug 4, 2026

Quality Gate failed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

MykolaSuperman Aug 2, 2026 •

edited

Loading

al1img Aug 3, 2026 •

edited

Loading

mykola-kobets-epam Aug 3, 2026 •

edited

Loading