Cellular: fix ATHandler destructor possible crash on delete #10684

jarvte · 2019-05-28T10:01:52Z

Description

In some multithread cases there is possibility that process_oob function was called after ATHandler was deleted. Fix is to cancel eventqueue if it's holding an event and wait if oob processing is ongoing.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@kjbracey-arm @mirelachirica

Release Notes

ciarmcom · 2019-05-28T11:00:48Z

@jarvte, thank you for your changes.
@mirelachirica @kjbracey-arm @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

kjbracey · 2019-05-28T11:03:20Z

features/cellular/framework/AT/ATHandler.cpp

    set_file_handle(NULL);

+    // cancel possible oob event from queue and wait if oob is ongoing
+    _queue.cancel(_event_id);


If the cancel reports success, then you need to set oob_queued to false, no? Maybe make this conditional on oob_queued, to avoid stale event_id.

(Could _oob_queued be replaced by a null event_id check? Don't need separate variables?)

As cancel is void function we decided not to call it.
Replaced _oob_queued and _oob_pending with _event_id.
Force pushed changes.

equeue should be handling stale event_ids.

Each event gets its own generation count which is packed into the id. If the generation count doesn't match equeue_cancel is a noop.

equeue_cancel(0) is also a noop, much like free(NULL).

features/cellular/framework/AT/ATHandler.cpp

kjbracey · 2019-05-30T07:52:22Z

features/cellular/framework/AT/ATHandler.cpp

    set_file_handle(NULL);

+    while (_event_id > 0) {
+#ifdef AT_HANDLER_MUTEX


I think we are in a bit of a pickle here.

We can't tell if cancel worked, so if we do call it we don't know if we need to wait for event_id to be cleared. That's why it got removed.

But if the OOBs are running on the same thread as this destructor, which I believe is quite possible in some event-driven apps, we must call cancel (and we can assume it would work in that case). We can't wait - we'd deadlock the event queue.

As a minimal change, this could be

#ifdef AT_HANDLER_MUTEX while (_event_id > 0) { _oobCv.wait(); } #else if (_event_id > 0) { event_cancel(_event_id); } #endif

But that still isn't any good if using an event queue with AT_HANDLER_MUTEX defined.

Ideally I'd like a a success/error return from cancel. But I recall this was discussed before - @kivaisan was looking at it? I'm not sure if the conclusion was that it wasn't possible to get a solid return, or that a different solution was found.

Is this something that would be better fixed by using the event's destructor?
https://github.com/ARMmbed/mbed-os/blob/master/events/equeue/equeue.h#L161

This tells you when the event has expired, either by completing or being canceled.

It's indirectly exposed through the C++ API via function objects, though it's messy:

bool dead = false; class DeathHook { void operator()(void) { // do stuff } ~DeathHook() { dead = true; } }; int id = queue.call(DeathHook()); queue.cancel(id); printf("is dead? %d\n", dead);

Does that example work at present? You're passing a temporary by value, and that temporary gets copied then destroyed, so I think dead is set immediately after the call().

I think it could work if EventQueue was upgraded to use C++11 perfect forwarding. Or, more simply, if the constructor/destructor reference counted.

You know, I actually have no idea. I think you're right this would need reference counting to work.

Alternatively we could poke an explicit destructor-register function through the C++ API like in the C one.

Or you might be able to extend the Event class.

features/cellular/framework/AT/ATHandler.cpp

AriParkkila · 2019-06-05T10:42:25Z

features/cellular/framework/AT/ATHandler.h

@@ -212,7 +213,8 @@ class ATHandler {
 protected:
    void event();
 #ifdef AT_HANDLER_MUTEX
-    PlatformMutex _fileHandleMutex;
+    rtos::Mutex _fileHandleMutex;
+    rtos::ConditionVariable _oobCv;


The purpose of _oobCv is unclear to me, could add here some description. If _oobCv is supposed to guard _event_id then event() and unlock() need locks.

oobCv notifies oob completion (it's notified when event_id becomes -1).

I agree that it is assuming event_id is protected by the _fileHandleMutex. That is held as unlock is entered, so access there is fine. If event() can be called without that held, then yes it should have a lock.

event is actually called from IRQ context, which means mutex is not possible. But it also means it's pretty safe - this is no worse than the previous _oob_queued version, at least. Suggest not changing at this point.

features/cellular/framework/AT/ATHandler.cpp

0xc0170 · 2019-08-14T09:19:06Z

@jarvte Any update for this PR, is this for 5.14?

jarvte · 2019-08-14T09:30:29Z

@jarvte Any update for this PR, is this for 5.14?

Yes, for 5.14. No time to do this now.

0xc0170 · 2019-08-26T11:43:41Z

5.14 is closing soon, and this has not seen any update. Should we keep the label here or this shall be closed as "will be updated later and reopened" ?

0xc0170 · 2019-08-29T07:07:00Z

@mirelachirica @jarvte We need reviews here and the status. If no update made by later today, we'll remove 5.14 label and move this to the next minor version. We are close to the code freeze.
Let us know

mirelachirica · 2019-08-29T11:48:35Z

I have rebased the branch and it builds/passes now UT. What comes to rest of the content and changes to ATHandler, they need approval from @kjbracey-arm, @geky, @AnttiKauppila.

kjbracey · 2019-08-29T11:59:15Z

Couple of my and @geky's comments are still outstanding - use the callback helper to simplify code; use 0 as invalid event-id, not positive/negative comparisons.

0xc0170 · 2019-08-30T12:39:22Z

Removed 5.14 label, can move to the next patch release.

jarvte · 2019-08-30T12:41:34Z

we are investigating. unittests were executed @jarvte ? Looks like they fail but can't spot the failure.

@0xc0170 I made the original pr but @mirelachirica is working on this one now. @mirelachirica ?

mirelachirica · 2019-08-30T12:43:54Z

we are investigating. unittests were executed @jarvte ? Looks like they fail but can't spot the failure.

I have executed UT localy and they build and pass. I do not see what is the failure here.

mirelachirica · 2019-08-30T13:53:41Z

It is a valgrind error. I will check it.

==12455== Memcheck, a memory error detector
==12455== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==12455== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==12455== Command: ./features-cellular-framework-AT-athandler -o junit
==12455==
==12455== Conditional jump or move depends on uninitialised value(s)
==12455== at 0x1D055A: mbed::ATHandler::unlock() (ATHandler.cpp:376)
...
==12455== Uninitialised value was created by a stack allocation
==12455== at 0x1AD76B: TestATHandler_test_ATHandler_cmd_start_Test::TestBody() (athandlertest.cpp:352)

==12455== For counts of detected and suppressed errors, rerun with: -v
==12455== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 0 from 0)

kjbracey · 2019-08-30T13:59:11Z

~~_recv_pos and _recv_len aren't reset on construction.~~

Yes they are, via a reset_buffer. Hmm. Next...

mirelachirica · 2019-09-02T09:58:52Z

Valgrind errors for athandler unittest are now fixed.

0xc0170 · 2019-09-02T12:30:12Z

CI restarted

mbed-ci · 2019-09-02T13:48:22Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 6
Build artifacts

0xc0170 · 2019-09-02T13:54:32Z

UNITTESTS/stubs/rtx_mutex_stub.c

@@ -0,0 +1,11 @@
+#include "cmsis_os2.h"


code files need license on top of them, please add to all new files

Added license where it was missing.

In some multithread cases there is possibility that process_oob function was called after ATHandler was deleted. Fix is to wait if oob processing is ongoing.

0xc0170 · 2019-09-03T08:44:42Z

CI started

mbed-ci · 2019-09-03T13:04:58Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 7
Build artifacts

ciarmcom requested review from kjbracey, mirelachirica and a team May 28, 2019 11:00

ciarmcom added the needs: review label May 28, 2019

kjbracey reviewed May 28, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels May 28, 2019

jarvte force-pushed the fix_athandler_destr branch from 4eb3665 to 2b2312d Compare May 29, 2019 10:18

kjbracey reviewed May 30, 2019

View reviewed changes

features/cellular/framework/AT/ATHandler.cpp Outdated Show resolved Hide resolved

kjbracey reviewed May 30, 2019

View reviewed changes

features/cellular/framework/AT/ATHandler.cpp Outdated Show resolved Hide resolved

kjbracey mentioned this pull request Jun 4, 2019

Changed EventQueue::cancel to return boolean value #10750

Merged

AriParkkila reviewed Jun 5, 2019

View reviewed changes

geky reviewed Jun 6, 2019

View reviewed changes

features/cellular/framework/AT/ATHandler.cpp Outdated Show resolved Hide resolved

0xc0170 added the release-version: 5.14.0-rc1 label Aug 14, 2019

mirelachirica force-pushed the fix_athandler_destr branch 2 times, most recently from f318f60 to 40faf90 Compare August 28, 2019 07:34

0xc0170 added needs: review and removed needs: work labels Aug 29, 2019

mirelachirica force-pushed the fix_athandler_destr branch from 40faf90 to 012e1a6 Compare August 29, 2019 11:35

0xc0170 requested review from kjbracey and AnttiKauppila August 29, 2019 11:50

0xc0170 removed the release-version: 5.14.0-rc1 label Aug 30, 2019

0xc0170 added needs: work and removed needs: CI labels Aug 30, 2019

mirelachirica force-pushed the fix_athandler_destr branch from d8aafd8 to 5b61b5c Compare September 2, 2019 09:54

0xc0170 added needs: CI and removed needs: work labels Sep 2, 2019

0xc0170 requested changes Sep 2, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Sep 2, 2019

Cellular: fix ATHandler destructor possible crash on delete

753ba8c

In some multithread cases there is possibility that process_oob function was called after ATHandler was deleted. Fix is to wait if oob processing is ongoing.

mirelachirica force-pushed the fix_athandler_destr branch from 5b61b5c to 753ba8c Compare September 3, 2019 06:10

0xc0170 approved these changes Sep 3, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Sep 3, 2019

0xc0170 added ready for merge release-version: 5.14.1 and removed needs: CI labels Sep 3, 2019

0xc0170 merged commit aba0760 into ARMmbed:master Sep 4, 2019

0xc0170 removed the ready for merge label Sep 4, 2019

jarvte deleted the fix_athandler_destr branch January 17, 2020 12:21

Cellular: fix ATHandler destructor possible crash on delete #10684

Cellular: fix ATHandler destructor possible crash on delete #10684

Uh oh!

Conversation

jarvte commented May 28, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented May 28, 2019

Uh oh!

kjbracey May 28, 2019 • 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Jun 6, 2019 • 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!

Uh oh!

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!

Uh oh!

0xc0170 commented Aug 14, 2019

Uh oh!

jarvte commented Aug 14, 2019

Uh oh!

0xc0170 commented Aug 26, 2019

Uh oh!

0xc0170 commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirelachirica commented Aug 29, 2019

Uh oh!

kjbracey commented Aug 29, 2019

Uh oh!

0xc0170 commented Aug 30, 2019

Uh oh!

jarvte commented Aug 30, 2019

Uh oh!

mirelachirica commented Aug 30, 2019

Uh oh!

mirelachirica commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirelachirica commented Sep 2, 2019

Uh oh!

0xc0170 commented Sep 2, 2019

Uh oh!

mbed-ci commented Sep 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Sep 3, 2019

Uh oh!

mbed-ci commented Sep 3, 2019

kjbracey May 28, 2019 •

edited

Loading

geky Jun 6, 2019 •

edited

Loading

0xc0170 commented Aug 29, 2019 •

edited

Loading

mirelachirica commented Aug 30, 2019 •

edited

Loading

kjbracey commented Aug 30, 2019 •

edited

Loading