cpu/esp32: fixes for boot issues and crashes on ESP32 #19192

Flole998 · 2023-01-24T16:52:17Z

Contribution description

In syscalls_init() there is a call to malloc(), which will return NULL if the heap is not initialized before, causing the entire board to fail booting if MODULE_ESP_IDF_HEAP is used.

API of vTaskDelete() says, that if NULL is passed to vTaskDelete the calling task should be deleted.

This PR needs a backport to 2023.01.

Testing procedure

Just compiling on the ESP32S2 and running it with WiFi caused it to not start anymore, no output, nothing. When Null is written to that null-pointer it hangs.

The second commit fixed an issue/assertion fail that happens when the WiFi connection drops/disconnects.

Issues/PRs references

Issue was introduced with PR #19146

In syscalls_init() there is a call to malloc(), which will return NULL if the heap is not initialized before, causing the entire board to fail booting if MODULE_ESP_IDF_HEAP is used.

cpu/esp_common/freertos/task.c

riot-ci · 2023-01-24T18:07:54Z

Murdock results

✔️ PASSED

56d2997 cpu/esp_common/freertos: Handle NULL being passed to vTaskDelete

Success	Failures	Total	Runtime
6796	0	6796	11m:44s

Artifacts

Documentation preview

gschorcht · 2023-01-24T19:31:00Z

Oops, thank you for figuring out that issue. It is is a side effect of the recent change d5a28ec in PR #19146 where syscalls_init moved to an earlier position in the startup procedure to solve another issue. Since we tested it without WiFi, the esp_idf_heap module wasn't used and I didn't realize this side effect.

I'm not sure whether calling esp_idf_heap_init might cause other problems, I have to check. I'm also wondering whether we really need that malloc in syscalls_init

RIOT/cpu/esp_common/syscalls.c

Lines 534 to 535 in f85366a

    
           environ = malloc(sizeof(char*)); 
        
           environ[0] = NULL;

is needed at all or whether the environment variable could also be static variable initialized with a static initializer.

Flole998 · 2023-01-24T19:33:40Z

I went through the code to check for potential other issues but haven't seen anything that could potentially break.

I guess environ is realloc()ed if it's resized, or at least that's what I would expect. So static would not work.

gschorcht · 2023-01-24T19:34:24Z

I guess environ is realloc()ed if it's resized, or at least that's what I would expect. So static would not work.

Ok, then it has to be dynamic.

gschorcht · 2023-01-24T19:35:22Z

@Flole998 Could you add some hints or just a make command how the issue can be reproduced and tested in your description?

Flole998 · 2023-01-24T19:35:40Z

I guess environ is realloc()ed if it's resized, or at least that's what I would expect. So static would not work.

Ok, then it has to be dynamic.

That's just a wild guess, I haven't checked but allocating a single char buffer indicates that.

Flole998 · 2023-01-24T19:38:01Z

@Flole998 Could you add some hints or just a make command how the issue can be reproduced and tested in your description?

Just compiling on the ESP32S2 and running it with WiFi caused it to not start anymore, no output, nothing. When Null is written to that null-pointer it hangs.

The second commit fixed an issue that happens when the WiFi connection drops/disconnects.

gschorcht · 2023-01-24T19:53:24Z

The second commit fixed an issue/assertion fail that happens when the WiFi connection drops/disconnects.

Are you sure, I haven't seen that? Anyway, it seems to be a bug.

Flole998 · 2023-01-24T19:54:47Z

The second commit fixed an issue/assertion fail that happens when the WiFi connection drops/disconnects.

Are you sure, I haven't seen that? Anyway, it seems to be a bug.

Yes, absolutely sure. It might only happen on WPA Enterprise connections though.

gschorcht

Code looks good, works as expected.

Flole998 · 2023-01-24T19:58:53Z

@gschorcht It looks like you accidentally added a wrong "Fixes:" entry right at the beginning of the description, could you double check that? The issue seems unrelated

gschorcht · 2023-01-24T20:03:52Z

bors merge

19192: cpu/esp32: fixes for boot issues and crashes on ESP32 r=gschorcht a=Flole998 ### Contribution description In syscalls_init() there is a call to malloc(), which will return NULL if the heap is not initialized before, causing the entire board to fail booting if MODULE_ESP_IDF_HEAP is used. API of [vTaskDelete()](https://www.freertos.org/a00126.html) says, that if NULL is passed to vTaskDelete the calling task should be deleted. This PR needs a backport to 2023.01. ### Testing procedure Just compiling on the ESP32S2 and running it with WiFi caused it to not start anymore, no output, nothing. When Null is written to that null-pointer it hangs. The second commit fixed an issue/assertion fail that happens when the WiFi connection drops/disconnects. ### Issues/PRs references Issue was introduced with PR #19146 Co-authored-by: Flole998 <[email protected]>

kaspar030 · 2023-01-24T20:36:15Z

bors cancel
bors merge

bors · 2023-01-24T20:36:17Z

Canceled.

bors · 2023-01-24T23:29:21Z

Build succeeded:

gschorcht · 2023-01-25T14:34:59Z

@Flole998 Thanks for providing the bug fix.

19196: cpu/esp32: fixes for boot issues and crashes on ESP32 [backport 2023.1] r=benpicco a=gschorcht # Backport of PR #19192 ### Contribution description In `syscalls_init()` there is a call to `malloc()`, which will return `NULL` if the heap is not initialized before, causing the entire board to fail booting if module `esp_idf_heap` is used, for example if the WiFi network interface is used. API of [vTaskDelete()](https://www.freertos.org/a00126.html) says, that if NULL is passed to vTaskDelete the calling task should be deleted. This PR needs a backport to 2023.01. ### Testing procedure ``` USEMODULE=esp_idf_heap BOARD=esp32-wroom-32 make -C tests/malloc ```` should work now but hangs in startup without the PR. ### Issues/PRs references Issue was introduced with PR #19146 Co-authored-by: Flole998 <[email protected]>

cpu/esp32: Make sure heap is initialized early enough

1a4af03

In syscalls_init() there is a call to malloc(), which will return NULL if the heap is not initialized before, causing the entire board to fail booting if MODULE_ESP_IDF_HEAP is used.

Flole998 requested a review from gschorcht as a code owner January 24, 2023 16:52

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Jan 24, 2023

Flole998 force-pushed the esp_malloc_fix branch from 5dfae81 to 1319297 Compare January 24, 2023 16:53

benpicco reviewed Jan 24, 2023

View reviewed changes

cpu/esp_common/freertos/task.c Outdated Show resolved Hide resolved

cpu/esp_common/freertos: Handle NULL being passed to vTaskDelete

56d2997

Flole998 force-pushed the esp_malloc_fix branch from 1319297 to 56d2997 Compare January 24, 2023 17:48

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 24, 2023

gschorcht changed the title ~~Fixes for boot issues and crashes on ESP32~~ cpu/esp32: fixes for boot issues and crashes on ESP32 Jan 24, 2023

gschorcht approved these changes Jan 24, 2023

View reviewed changes

bors bot merged commit 719787b into RIOT-OS:master Jan 24, 2023

gschorcht mentioned this pull request Jan 25, 2023

cpu/esp32: fixes for boot issues and crashes on ESP32 [backport 2023.1] #19196

Merged

gschorcht added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jan 25, 2023

MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023

cpu/esp32: fixes for boot issues and crashes on ESP32 #19192

cpu/esp32: fixes for boot issues and crashes on ESP32 #19192

Uh oh!

Conversation

Flole998 commented Jan 24, 2023 • edited by gschorcht Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

riot-ci commented Jan 24, 2023

Murdock results

Artifacts

Uh oh!

gschorcht commented Jan 24, 2023

Uh oh!

Flole998 commented Jan 24, 2023

Uh oh!

gschorcht commented Jan 24, 2023

Uh oh!

gschorcht commented Jan 24, 2023

Uh oh!

Flole998 commented Jan 24, 2023

Uh oh!

Flole998 commented Jan 24, 2023

Uh oh!

gschorcht commented Jan 24, 2023

Uh oh!

Flole998 commented Jan 24, 2023

Uh oh!

gschorcht left a comment

Choose a reason for hiding this comment

Uh oh!

Flole998 commented Jan 24, 2023

Uh oh!

gschorcht commented Jan 24, 2023

Uh oh!

kaspar030 commented Jan 24, 2023

Uh oh!

bors bot commented Jan 24, 2023

Uh oh!

bors bot commented Jan 24, 2023

Uh oh!

gschorcht commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Flole998 commented Jan 24, 2023 •

edited by gschorcht

Loading