Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe build matrix adds Tone and TodoList. The change adds Tone’s CMake configuration and manifest. The Tone app provides four frequency presets, including a repeating sweep, and generates 48 kHz, 16-bit stereo audio. Its interface provides playback controls, a volume slider, and playback status. The app reports device, stream, task, and write failures. On close, it stops playback and waits for the playback task to exit. Priority: ⬇️ Low Merge Risk: 🟠 High · up to Closing the Tone app during playback can make the audio task touch memory that has already been freed and crash the device. Pressing Play again while playback is still stopping can break the new playback session. Fix both before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Playback can overlap during a quick stop and restart, and closing the app may finish before its playback task has stopped using app-owned state. These are locally triggered risks; broader device impact is not established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 1 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: a05f571a-0d0d-44e7-aabd-73562038d3b6
📒 Files selected for processing (5)
.github/workflows/main.ymlApps/Tone/CMakeLists.txtApps/Tone/main/CMakeLists.txtApps/Tone/main/Source/App.cppApps/Tone/manifest.properties
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| audio_stream_close(ctx->streamHandle); | ||
| ctx->streamHandle = nullptr; | ||
| ctx->playbackTask = nullptr; | ||
|
|
||
| lvgl_lock(); | ||
| updateStatusLabel(ctx); | ||
| updatePlayButton(ctx); | ||
| lvgl_unlock(); | ||
|
|
||
| vTaskDelete(nullptr); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Clear playbackTask only after the task stops using ctx.
The task sets ctx->playbackTask = nullptr at Line 188. After that, it calls lvgl_lock() and updates widgets through ctx. On close, main stops waiting as soon as playbackTask is null. It then calls window_manager_remove and returns, which destroys the stack-allocated Context ctx. The task can then read freed ctx data and deleted LVGL widgets. This is a use-after-free. playbackTask is also a plain field that two tasks share without synchronization.
Make the handshake an atomic flag. Set it as the last step before vTaskDelete. Skip the UI update when the app is closing.
🐛 Proposed fix
- TaskHandle_t playbackTask = nullptr;
+ TaskHandle_t playbackTask = nullptr;
+ std::atomic<bool> taskRunning { false };
+ std::atomic<bool> closing { false }; audio_stream_close(ctx->streamHandle);
ctx->streamHandle = nullptr;
- ctx->playbackTask = nullptr;
-
- lvgl_lock();
- updateStatusLabel(ctx);
- updatePlayButton(ctx);
- lvgl_unlock();
+ if (!ctx->closing.load()) {
+ lvgl_lock();
+ updateStatusLabel(ctx);
+ updatePlayButton(ctx);
+ lvgl_unlock();
+ }
+ ctx->playbackTask = nullptr;
+ ctx->taskRunning.store(false); // last access to ctx
vTaskDelete(nullptr);- ctx.playing.store(false);
- while (ctx.playbackTask != nullptr) {
+ ctx.closing.store(true);
+ ctx.playing.store(false);
+ while (ctx.taskRunning.load()) {
vTaskDelay(pdMS_TO_TICKS(10));
}Before calling xTaskCreate, set taskRunning to true. If task creation fails, set it back to false.
Also applies to: 414-419
| if (ctx->playing.load()) { | ||
| ctx->playing.store(false); | ||
| lv_label_set_text(ctx->statusLabel, "Stopping..."); | ||
| updatePlayButton(ctx); | ||
| return; | ||
| } | ||
|
|
||
| Device* streamDevice = nullptr; | ||
| if (device_get_first_by_type(&AUDIO_STREAM_TYPE, &streamDevice) != ERROR_NONE || streamDevice == nullptr) { | ||
| lv_label_set_text(ctx->statusLabel, "No audio stream device"); | ||
| return; | ||
| } | ||
| ctx->streamDevice = streamDevice; | ||
|
|
||
| const AudioStreamConfig config = { | ||
| .sample_rate = SAMPLE_RATE, | ||
| .bits_per_sample = BITS_PER_SAMPLE, | ||
| .channels = CHANNELS, | ||
| }; | ||
|
|
||
| AudioStreamHandle handle = nullptr; | ||
| if (audio_stream_open_output(streamDevice, &config, &handle) != ERROR_NONE) { | ||
| lv_label_set_text(ctx->statusLabel, "Failed to open output stream"); | ||
| return; | ||
| } | ||
|
|
||
| ctx->streamHandle = handle; | ||
| ctx->playing.store(true); | ||
| BaseType_t taskResult = xTaskCreate(playbackTask, "tone-playback", PLAYBACK_TASK_STACK_BYTES, ctx, PLAYBACK_TASK_PRIORITY, &ctx->playbackTask); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Block Play while the previous playback task is still stopping.
Pressing Stop only sets playing to false. The old task can still be blocked in audio_stream_write. If the user presses Play during this window, onPlayPressed opens a new stream. It then overwrites ctx->streamHandle and ctx->playbackTask. When the old task exits, it closes ctx->streamHandle at Line 186. That handle is now the new stream, so the new task writes to a closed handle. The new stream from Line 222 is closed early, and the old stream is closed twice or its handle is lost. The old task also clears ctx->playbackTask for the new task. The close wait at Line 416 can then return while the new task is still running.
Return early while a task still exists. Pass the stream handle to the task as its own copy.
🐛 Proposed fix
if (ctx->playing.load()) {
ctx->playing.store(false);
lv_label_set_text(ctx->statusLabel, "Stopping...");
updatePlayButton(ctx);
return;
}
+ if (ctx->playbackTask != nullptr) {
+ return; // previous task is still shutting down
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ctx->playing.load()) { | |
| ctx->playing.store(false); | |
| lv_label_set_text(ctx->statusLabel, "Stopping..."); | |
| updatePlayButton(ctx); | |
| return; | |
| } | |
| Device* streamDevice = nullptr; | |
| if (device_get_first_by_type(&AUDIO_STREAM_TYPE, &streamDevice) != ERROR_NONE || streamDevice == nullptr) { | |
| lv_label_set_text(ctx->statusLabel, "No audio stream device"); | |
| return; | |
| } | |
| ctx->streamDevice = streamDevice; | |
| const AudioStreamConfig config = { | |
| .sample_rate = SAMPLE_RATE, | |
| .bits_per_sample = BITS_PER_SAMPLE, | |
| .channels = CHANNELS, | |
| }; | |
| AudioStreamHandle handle = nullptr; | |
| if (audio_stream_open_output(streamDevice, &config, &handle) != ERROR_NONE) { | |
| lv_label_set_text(ctx->statusLabel, "Failed to open output stream"); | |
| return; | |
| } | |
| ctx->streamHandle = handle; | |
| ctx->playing.store(true); | |
| BaseType_t taskResult = xTaskCreate(playbackTask, "tone-playback", PLAYBACK_TASK_STACK_BYTES, ctx, PLAYBACK_TASK_PRIORITY, &ctx->playbackTask); | |
| if (ctx->playing.load()) { | |
| ctx->playing.store(false); | |
| lv_label_set_text(ctx->statusLabel, "Stopping..."); | |
| updatePlayButton(ctx); | |
| return; | |
| } | |
| if (ctx->playbackTask != nullptr) { | |
| return; // previous task is still shutting down | |
| } | |
| Device* streamDevice = nullptr; | |
| if (device_get_first_by_type(&AUDIO_STREAM_TYPE, &streamDevice) != ERROR_NONE || streamDevice == nullptr) { | |
| lv_label_set_text(ctx->statusLabel, "No audio stream device"); | |
| return; | |
| } | |
| ctx->streamDevice = streamDevice; | |
| const AudioStreamConfig config = { | |
| .sample_rate = SAMPLE_RATE, | |
| .bits_per_sample = BITS_PER_SAMPLE, | |
| .channels = CHANNELS, | |
| }; | |
| AudioStreamHandle handle = nullptr; | |
| if (audio_stream_open_output(streamDevice, &config, &handle) != ERROR_NONE) { | |
| lv_label_set_text(ctx->statusLabel, "Failed to open output stream"); | |
| return; | |
| } | |
| ctx->streamHandle = handle; | |
| ctx->playing.store(true); | |
| BaseType_t taskResult = xTaskCreate(playbackTask, "tone-playback", PLAYBACK_TASK_STACK_BYTES, ctx, PLAYBACK_TASK_PRIORITY, &ctx->playbackTask); |
|
Neat! |
|
Seems like I broke the build. I'll fix it. |
|
@NellowTCS
edit: I was able to resolve it myself. |
Signed-off-by: Ken Van Hoeylandt <git@kenvanhoeylandt.net>
2 participants
Summary by CodeRabbit