DMalloc detection of free space overwrite during TestRead #22160
Labels
Comments
|
That log line is doing: and So in this case |
|
Oh, and I expect the "free space has been overwritten" bit is from the immediate next line, which does: |
|
Did not yet manage to run in RR, however only on my intel vm after a few runs I managed to get a coredump: |
|
Got a RR trace, investigating. For now the trace there seems different: |
|
ReadHandler seems to be cleared and the free stack trace according to RR is: with usage afterwards in: |
|
Note: in OnUnblockHoldReportCallback, the object is obviously freed and cleared: |
|
I suspect the key is this bit in the destructor stack: So unlike the "normal" non-test case this message is coming off a timer. And then it's very possible that we're hitting #19387 "can't cancel a timer if another timer is firing and the timers were scheduled for close together" issue. |
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this issue
Sep 2, 2022
To minimize risk, the changes here keep the "grab all the timers we should fire, then fire them" setup instead of switching to the "fire the timers one at a time" approach LayerImplFreeRTOS uses. The fix consists of the following parts: 1) Store the list of timers to fire in a member, so we can cancel things from that list as needed. 2) To avoid canceling things scheduled by ScheduleWork, remove the CancelTimer call in ScheduleWork. This does mean we now allow multiple timers for the same callback+appState in the timer list, if they were created by ScheduleWork, but that should be OK, since the only reason that pair needs to be unique is to allow cancellation and we never want to cancel the things ScheduleWork schedules. As a followup we should stop using the timer list for ScheduleWork altogether and use ScheduleLambda like LayerImplFreeRTOS does, but that involves fixing the unit tests that call ScheduleWork without actually running the platfor manager event loop and expect it to work somehow. TestRead was failing the sanity assert for not losing track of timers to fire, because it was spinning a nested event loop. The changes to that test stop it from doing that. Fixes project-chip#19387 Fixes project-chip#22160
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this issue
Sep 3, 2022
To minimize risk, the changes here keep the "grab all the timers we should fire, then fire them" setup instead of switching to the "fire the timers one at a time" approach LayerImplFreeRTOS uses. The fix consists of the following parts: 1) Store the list of timers to fire in a member, so we can cancel things from that list as needed. 2) To avoid canceling things scheduled by ScheduleWork, remove the CancelTimer call in ScheduleWork. This does mean we now allow multiple timers for the same callback+appState in the timer list, if they were created by ScheduleWork, but that should be OK, since the only reason that pair needs to be unique is to allow cancellation and we never want to cancel the things ScheduleWork schedules. As a followup we should stop using the timer list for ScheduleWork altogether and use ScheduleLambda like LayerImplFreeRTOS does, but that involves fixing the unit tests that call ScheduleWork without actually running the platfor manager event loop and expect it to work somehow. TestRead was failing the sanity assert for not losing track of timers to fire, because it was spinning a nested event loop. The changes to that test stop it from doing that. Fixes project-chip#19387 Fixes project-chip#22160
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this issue
Sep 3, 2022
To minimize risk, the changes here keep the "grab all the timers we should fire, then fire them" setup instead of switching to the "fire the timers one at a time" approach LayerImplFreeRTOS uses. The fix consists of the following parts: 1) Store the list of timers to fire in a member, so we can cancel things from that list as needed. 2) To avoid canceling things scheduled by ScheduleWork, remove the CancelTimer call in ScheduleWork. This does mean we now allow multiple timers for the same callback+appState in the timer list, if they were created by ScheduleWork, but that should be OK, since the only reason that pair needs to be unique is to allow cancellation and we never want to cancel the things ScheduleWork schedules. As a followup we should stop using the timer list for ScheduleWork altogether and use ScheduleLambda like LayerImplFreeRTOS does, but that involves fixing the unit tests that call ScheduleWork without actually running the platfor manager event loop and expect it to work somehow. TestRead was failing the sanity assert for not losing track of timers to fire, because it was spinning a nested event loop. The changes to that test stop it from doing that. Fixes project-chip#19387 Fixes project-chip#22160
andy31415
added a commit
that referenced
this issue
Sep 5, 2022
* Duplicate src/system/tests/TestSystemScheduleLambda.cpp history in src/system/tests/TestSystemScheduleWork.cpp history. * Fix timer cancellation to be reliable in LayerImplSelect. To minimize risk, the changes here keep the "grab all the timers we should fire, then fire them" setup instead of switching to the "fire the timers one at a time" approach LayerImplFreeRTOS uses. The fix consists of the following parts: 1) Store the list of timers to fire in a member, so we can cancel things from that list as needed. 2) To avoid canceling things scheduled by ScheduleWork, remove the CancelTimer call in ScheduleWork. This does mean we now allow multiple timers for the same callback+appState in the timer list, if they were created by ScheduleWork, but that should be OK, since the only reason that pair needs to be unique is to allow cancellation and we never want to cancel the things ScheduleWork schedules. As a followup we should stop using the timer list for ScheduleWork altogether and use ScheduleLambda like LayerImplFreeRTOS does, but that involves fixing the unit tests that call ScheduleWork without actually running the platfor manager event loop and expect it to work somehow. TestRead was failing the sanity assert for not losing track of timers to fire, because it was spinning a nested event loop. The changes to that test stop it from doing that. Fixes #19387 Fixes #22160 * Add a unit test that timer cancelling works even for currently "in progress" timers * Address review comments. * Fix shadowing problem. * Turn off TestSystemScheduleWork on esp32 QEMU for now. * Turn off TestSystemScheduleWork on the fake platform too. The fake platform's event loop does not actually process the SystemLayer events. Co-authored-by: Andrei Litvin <[email protected]>
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this issue
Sep 16, 2022
…ip#22375) * Duplicate src/system/tests/TestSystemScheduleLambda.cpp history in src/system/tests/TestSystemScheduleWork.cpp history. * Fix timer cancellation to be reliable in LayerImplSelect. To minimize risk, the changes here keep the "grab all the timers we should fire, then fire them" setup instead of switching to the "fire the timers one at a time" approach LayerImplFreeRTOS uses. The fix consists of the following parts: 1) Store the list of timers to fire in a member, so we can cancel things from that list as needed. 2) To avoid canceling things scheduled by ScheduleWork, remove the CancelTimer call in ScheduleWork. This does mean we now allow multiple timers for the same callback+appState in the timer list, if they were created by ScheduleWork, but that should be OK, since the only reason that pair needs to be unique is to allow cancellation and we never want to cancel the things ScheduleWork schedules. As a followup we should stop using the timer list for ScheduleWork altogether and use ScheduleLambda like LayerImplFreeRTOS does, but that involves fixing the unit tests that call ScheduleWork without actually running the platfor manager event loop and expect it to work somehow. TestRead was failing the sanity assert for not losing track of timers to fire, because it was spinning a nested event loop. The changes to that test stop it from doing that. Fixes project-chip#19387 Fixes project-chip#22160 * Add a unit test that timer cancelling works even for currently "in progress" timers * Address review comments. * Fix shadowing problem. * Turn off TestSystemScheduleWork on esp32 QEMU for now. * Turn off TestSystemScheduleWork on the fake platform too. The fake platform's event loop does not actually process the SystemLayer events. Co-authored-by: Andrei Litvin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
Two instances for this at least:
https://github.com/project-chip/connectedhomeip/runs/8005304886?check_suite_focus=true
https://github.com/project-chip/connectedhomeip/runs/7984389882?check_suite_focus=true
Log says:
I am unclear where
57311seconds come from (seems like an abnormally large seconds count) however timestamps do not seem to point at a large thread block either. Unfortunately TestRead in general is very slow to run and with a lot of logs, making it hard to debug.The text was updated successfully, but these errors were encountered: