llext: fix memory leak in no-data case#10599
llext: fix memory leak in no-data case#10599wjablon1 wants to merge 1 commit intothesofproject:mainfrom
Conversation
wjablon1
requested review from
abonislawski,
lyakh,
serhiy-katsyuba-intel,
softwarecki and
tmleman
wjablon1
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
src/library_manager/llext_manager.c
Outdated
| if (ret < 0) | ||
| err = ret; | ||
|
|
||
| va_base_data = va_base_data ? va_base_data : va_bss_data; |
There was a problem hiding this comment.
I would inline comment this assumption to make it easier to follow.
There was a problem hiding this comment.
I really don't like redundancy like
if (x)
x = x;
else
x = y;
Can we just make this
if (!va_base_data)
va_base_data = va_base_bss;
or maybe even use size:
if (!mctx->segment[LIB_MANAGER_DATA].size)
va_base_data = va_base_bss;
There was a problem hiding this comment.
OK, will fix. It's just my habit from the previous project where constructions like this were common and immediately understood. I need to get rid of this habit :/
There was a problem hiding this comment.
@wjablon1 no worries, the compiler would probably optimise that away, but... it just looks not very pretty to me, sorry, just my personal preference :-)
There was a problem hiding this comment.
@lyakh I agree that in this case your proposal looks much cleaner if you consider all the other SOF stylistic rules. But in other projects it's forbidden to omit curly brackets with if statements so the construction I used was a handy shortcut.
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
thanks for the fix! Looks correct, can we just improve cosmetics a bit
src/library_manager/llext_manager.c
Outdated
| /* Writable data (.data, .bss, etc.) */ | ||
| void __sparse_cache *va_base_data = (void __sparse_cache *) | ||
| mctx->segment[LIB_MANAGER_DATA].addr; | ||
| void __sparse_cache *va_bss_data = (void __sparse_cache *) |
There was a problem hiding this comment.
for consistency this should be called va_base_bss or bss_addr to match llext_manager_load_module().
src/library_manager/llext_manager.c
Outdated
| if (ret < 0) | ||
| err = ret; | ||
|
|
||
| va_base_data = va_base_data ? va_base_data : va_bss_data; |
There was a problem hiding this comment.
I really don't like redundancy like
if (x)
x = x;
else
x = y;
Can we just make this
if (!va_base_data)
va_base_data = va_base_bss;
or maybe even use size:
if (!mctx->segment[LIB_MANAGER_DATA].size)
va_base_data = va_base_bss;
If there is no .data section in a llext module, the base data address is replaced with the .bss address while loading the module. There is no a corresponding logic during module unloading. This causes a memory leak because FW attempts to free memory under a NULL address while the .bss memory remains allocated. This change adds the missing logic. Signed-off-by: Wojciech Jablonski <wojciech.jablonski@intel.com>
6 participants
If there is no .data section in a llext module, the base data address is replaced with the .bss address while loading the module. There is no a corresponding logic during module unloading. This causes a memory leak because FW attempts to free memory under a NULL address while the .bss memory remains allocated. This change adds the missing logic.
The issue was caught with RTC_AEC module during long run/multi-test or immediate Zephyr assert with debug build