Resolve unsafe memory accesses in ExchangeContext (#25023) #25032
Conversation
|
|
pullapprove
bot
requested review from
amitnj,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
CodeChronos928,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
jtung-apple,
kcoppock,
ksperling-apple,
lazarkov,
lpbeliveau-silabs and
LuDuda
pullapprove
bot
requested review from
mlepage-google,
mrjerryjohns,
msandstedt,
mspang,
pjzander-signify,
robszewczyk,
saurabhst,
selissia,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
woody-apple,
xylophone21 and
yufengwangca
|
PR #25032: Size comparison from e7528bc to 6bac357 Increases (1 build for cc32xx)
Full report (1 build for cc32xx)
|
| // Verify that the Session Manager is still alive. Note that host applications may stop the chip::Server | ||
| // at any time, which will tear down the Session Manager, Exchange Manager, and Exchange Context. Note |
There was a problem hiding this comment.
No, they're not allowed to stop it at any time, unless they are holding the Matter stack lock. We should add an assert to that effect.
There was a problem hiding this comment.
Can you share any references to this expectation in the codebase? It isn't enforced anywhere in ExchangeMgr or any other file in messaging/* except for a single usage in ExchangeContext at the beginning of SendMessage.
There was a problem hiding this comment.
Can you share any references to this expectation in the codebase? I
Which expectation? That you only touch the Matter SDK while holding the Matter lock? It's a basic API expectation in all Matter APIs. Not all of them assert this, but that's been a matter of code size and time. If you call Matter APIs that mutate state on multiple threads without locking, that cannot possibly be safe and will lead to data/memory corruption and hence bugs.
For what it's worth, see #25041
There was a problem hiding this comment.
And note that the lock asserts are no-ops on various platforms. Again, the expectation is that SDK consumers do the right thing and only touch SDK APIs with the locks held, either directly (which also does not work on some lock-free platforms) or via posting whatever they are doing to the Matter event loop.
There was a problem hiding this comment.
Which expectation? That you only touch the Matter SDK while holding the Matter lock? It's a basic API expectation in all Matter APIs.
It's surprising that this assertion would not be mentioned in all but one file in this module. As a newcomer, and having only read a few dozen files in the SDK, this is the only time that I've seen it mentioned. Is there a centralized document explaining this thread safety approach?
Again, the expectation is that SDK consumers do the right thing
This doesn't seem like a reasonable assumption for an SDK... public APIs should generally be defensive and assume that the caller can always make the wrong decision. Even if the SDK chooses not to fail gracefully (a valid choice), clients need immediate feedback to help them find bugs in their code. The current segmentation fault in the middle of SendMessage being serviced by a different thread doesn't obviously indicate that the client (the tv-casting-app) is doing something wrong.
There was a problem hiding this comment.
Is there a centralized document explaining this thread safety approach?
I don't know of one... @mrjerryjohns did something ever get written here?
To be clear, this is not great, but that's the state of things right now. Not enough hands....
public APIs should generally be defensive and assume that the caller can always make the wrong decision.
Sure. Again:
- Not all platforms support checking the locking correctness.
- Adding the asserts to all the places that should have them has nonzero cost both in developer time and final codesize on the platforms that do have such checking.
- We do not have a clear distinction between private and public APIs right now (an issue in its own right).
We have been adding the asserts in an ad-hoc manner to high-value places that would have a good chance of catching bugs, but something more systematic would be welcome, obviously.
The current segmentation fault in the middle of SendMessage being serviced by a different thread doesn't obviously indicate that the client (the tv-casting-app) is doing something wrong.
Completely agreed! I'm just saying that the right fix here is not in SendMessage; that's going to cover up a thread race that's likely corrupting data elsewhere, at least probabilistically. The right fix is to get rid of the thread race.
There was a problem hiding this comment.
Thanks for the good discussion on this issue, I agree that we should fix the thread race instead. With #25041 merged, we should start seeing assertion failures now in the tv-casting-app example which would help pinpoint the threading problem. I'm happy to close out this PR, I'll just wait a few days to see if anyone chimes in with pointers on the thread safety documentation. Thanks!
| // that at the time of writing (2023/02/09) the SessionManager pointer, if non-null, is guaranteed to be | ||
| // valid because it is allocated in the parent Server class. |
There was a problem hiding this comment.
Unless you're not even a Server....
| ExchangeContext * ec = ctx.NewExchangeToAlice(&mockSolicitedAppDelegate); | ||
|
|
||
| // Close the Exchange Context. | ||
| ec->Close(); |
There was a problem hiding this comment.
After this ec is a dangling pointer....
| ec->Close(); | ||
|
|
||
| // Call public APIs to verify that they do not crash or fail the test. | ||
| NL_TEST_ASSERT(inSuite, ec->StartResponseTimer() == CHIP_ERROR_INTERNAL); |
There was a problem hiding this comment.
This absolutely can crash. It's calling a method on a deleted object!
| * Tests that the Exchange Context APIs do not crash if delayed calls are made after the Exchange Context is | ||
| * closed. | ||
| */ | ||
| void CheckExchangeContextDoesNotCrashWhenDelayedCallsOccurAfterClose(nlTestSuite * inSuite, void * inContext) |
There was a problem hiding this comment.
Also, this test does not compile, and if it compiled would not get run. Please make sure to verify that tests fail without the fix and pass with it...
|
I'm going to close this pull request and create a new one that addresses the root cause. |
Fixes #25023
Problem
When an
ExchangeMgris shut down (ShutDown()), theSessionManageris set tonullptrand resources are released. However, if a childExchangeContextis still in use on another thread (e.g. in the middle ofSendMessage(...)), theExchangeContextusesmExchangeManager->GetSessionManager()without anynullsafety checks. This can cause segmentation faults when thenullSessionManageris dereferenced.Solution
Add
nullchecks on all usages of theSessionManager. Return error codes where appropriate / applicable, and fail gracefully / silently where it is safe to do so.