Fix double-free bugs on failure to send a write message. #13051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There were two separate bugs here: 1) For a group write, we were violating the contract for WriteClient::SendWriteRequest, which is that the caller must call Shutdown on failure but the WriteClient itself will do it on success (and call OnDone in the process). We were unconditionally calling Shutdown and OnDone inside SetWriteRequest, even on failure. 2) WriteInteraction was violating the contract for WriteClientHandle::SendWriteRequest, which is different: it always guarantees it will call OnDone. But the consumer was assuming that OnDone would only be called if SendWriteRequest returned success.
bzbarsky-apple
requested review from
mrjerryjohns,
yunhanw-google and
jepenven-silabs
pullapprove
bot
requested review from
andy31415,
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
hawk248,
holbrookt,
jelderton,
jmartinez-silabs,
LuDuda,
lzgrablic02,
msandstedt and
pan-apple
pullapprove
bot
requested review from
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple and
xylophone21
yunhanw-google
approved these changes
Dec 15, 2021
|
PR #13051: Size comparison from 89aff25 to 6b8aef9 Increases (14 builds for esp32, k32w, mbed, nrfconnect, qpg, telink)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
andy31415
approved these changes
Dec 15, 2021
mrjerryjohns
approved these changes
Dec 16, 2021
|
fast track: small delta, several checkmarks, PR up for sufficient time for cross timezone review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were two separate bugs here:
For a group write, we were violating the contract for
WriteClient::SendWriteRequest, which is that the caller must call
Shutdown on failure but the WriteClient itself will do it on success
(and call OnDone in the process). We were unconditionally calling
Shutdown and OnDone inside SetWriteRequest, even on failure.
WriteInteraction was violating the contract for
WriteClientHandle::SendWriteRequest, which is different: it always
guarantees it will call OnDone. But the consumer was assuming that
OnDone would only be called if SendWriteRequest returned success.
Problem
See above.
Change overview
See above.
Testing
Manually tried running the TestGroupMessaging yaml test on darwin. It fails to send the messages (for now) and that was triggering the double-frees. With these fixes those double-frees are gone.