Fixing few issues with BindingManager and issues with how clients used BindingManager #22133
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
Fixed: * Issue where BindingManager::EstablishConnection only called once * Prevented used after free * Prevented buffer overrun
mrjerryjohns
approved these changes
Aug 24, 2022
|
Justification for 1.0 inclusion: Fixes a use-after-free bug with Bindings which can/should crash. |
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
lazarkov,
LuDuda and
msandstedt
pullapprove
bot
requested review from
saurabhst,
selissia,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple and
xylophone21
tehampson
commented
Aug 24, 2022
|
PR #22133: Size comparison from 591b386 to bc56a4a Increases (7 builds for bl602, cc13x2_26x2, efr32, psoc6)
Decreases (4 builds for cc13x2_26x2, cyw30739, psoc6)
Full report (41 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6)
|
This allows multiple connection async establishments to multiple nodes.
|
PR #22133: Size comparison from 591b386 to 780e73b Increases (2 builds for nrfconnect)
Decreases (2 builds for nrfconnect)
Full report (2 builds for nrfconnect)
|
woody-apple
approved these changes
Aug 25, 2022
tehampson
commented
Aug 25, 2022
mrjerryjohns
approved these changes
Aug 25, 2022
|
PR #22133: Size comparison from 7870328 to 6c78c16 Increases (23 builds for bl602, cc13x2_26x2, esp32, linux, nrfconnect, psoc6, telink)
Decreases (20 builds for cc13x2_26x2, esp32, linux, nrfconnect, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
|
PR #22133: Size comparison from 3d7cc78 to 13e80a9 Increases (25 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, telink)
Decreases (20 builds for cc13x2_26x2, esp32, linux, nrfconnect, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
| if (mLastSessionEstablishmentError == CHIP_ERROR_NO_MEMORY) | ||
| { | ||
| // Release the least recently used entry | ||
| // TODO: Some reference counting mechanism shall be added the CASESessionManager | ||
| // so that other session clients don't get accidentally closed. | ||
| ScopedNodeId peerToRemove; | ||
| if (mPendingNotificationMap.FindLRUConnectPeer(peerToRemove) == CHIP_NO_ERROR) | ||
| { | ||
| mPendingNotificationMap.RemoveAllEntriesForNode(peerToRemove); |
There was a problem hiding this comment.
How does this help, though? If we got NO_MEMORY that means the SessionSetup pool is full, no? How else can we end up here? And messing with our notification map will not help with that, I don't think; we would have to cancel some session setups, which is what this code used to do when it was working with OperationalDeviceProxy.
This part needs some thought as to how exactly it should work. Is this what #22173 is about?
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
…d BindingManager (project-chip#22133) * Couple with BindingManager Fixed: * Issue where BindingManager::EstablishConnection only called once * Prevented used after free * Prevented buffer overrun * Restyle * Allocate callbacks in EstablishConnection This allows multiple connection async establishments to multiple nodes. * Address PR comments
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.
Problem
Change overview
Testing
scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone chip_config_network_layer_ble=false chip_build_libshell=truerm -f rm -f kvs1 kvs2 kvs3Terminal 1run./out/debug/standalone/chip-all-clusters-app --secured-device-port 72564 --KVS kvs1(port was picked by mashing keys)Terminal 2run./out/debug/standalone/chip-tool pairing onnetwork 1 20202021Terminal 3run./out/debug/standalone/chip-all-clusters-app --secured-device-port 72574 --KVS kvs2Terminal 2run./out/debug/standalone/chip-tool pairing onnetwork 2 20202021Terminal 4run./out/debug/standalone/chip-all-clusters-app --KVS kvs3Terminal 2run./out/debug/standalone/chip-tool pairing onnetwork 3 20202021Terminal 2run./out/debug/standalone/chip-tool binding write binding '[{"fabricIndex": 1, "node": 2, "endpoint": 1, "cluster": 6}, {"fabricIndex": 1, "node": 3, "endpoint": 1, "cluster": 6}]' 1 1Terminal 1runswitch onTerminal 3run./out/debug/standalone/chip-all-clusters-app --secured-device-port 72574 --KVS kvs2Terminal 4run./out/debug/standalone/chip-all-clusters-app --KVS kvs3Terminal 1runswitch on