Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ExchangeContext: Implement a safer verion Abort/Close #19775

Closed
wants to merge 1 commit into from

Conversation

kghost
Copy link
Contributor

@kghost kghost commented Jun 20, 2022

Problem

Fixes #19747 Use-after-free when aborting already-closed exchange

Change overview

  • Introduce safer version Abort and Close for ExchangeContext
    • Abort or Close can be called at anytime on a valid exchange which is not releasing.
  • Once Abort or Close is called, or OnExchangeClosing is triggered, the exchange is releasing
    • The application should lose its control to that exchange. It must null-out all pointers to that exchange
    • The exchange will be released sooner or later.

This change is based on following concepts

  • Exchange refcounts are managed by internal states rather than by applications.
  • Instead of manually manipulate refcounts, Applications manage Exchange states.
    • Only have 2 states now, Closed or not, identified by kFlagClosed.
    • The initial refcount is bound to "not closed state" (kFlagClosed is not set)

Note: Currently, there are 3 types of ref counts to an exchange

  • The initial value: controlled by kFlagClosed
  • The scoped ExchangeHandle in member functions of ExchangeContex
  • The ExchangeHandle in RetransTableEntry

Testing

Verified by unit-tests.

@github-actions
Copy link

github-actions bot commented Jun 20, 2022

PR #19775: Size comparison from 48606c6 to 432255d

Increases (39 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section 48606c6 432255d change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 659567 659663 96 0.0
.rodata 87087 87143 56 0.1
.text 572168 572208 40 0.0
all-clusters-minimal-app LP_CC2652R7 (read only) 648063 648159 96 0.0
.rodata 90407 90463 56 0.1
.text 557336 557376 40 0.0
lock-ftd LP_CC2652R7 (read only) 690531 690627 96 0.0
.rodata 99163 99219 56 0.1
.text 590884 590924 40 0.0
lock-mtd LP_CC2652R7 (read only) 639931 640035 104 0.0
.rodata 99043 99099 56 0.1
.text 540396 540444 48 0.0
pump-app LP_CC2652R7 (read only) 671467 671571 104 0.0
.rodata 87259 87315 56 0.1
.text 583724 583772 48 0.0
pump-controller-app LP_CC2652R7 (read only) 657359 657455 96 0.0
.rodata 83103 83159 56 0.1
.text 573776 573816 40 0.0
shell LP_CC2652R7 (read only) 688994 689098 104 0.0
.rodata 109658 109714 56 0.1
.text 579024 579072 48 0.0
cyw30739 ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 616130 616226 96 0.0
.app_xip_area 473684 473780 96 0.0
efr32 lighting-app BRD4161A (read only) 924060 924180 120 0.0
.text 924052 924172 120 0.0
BRD4161A+rpc (read only) 959748 959868 120 0.0
.text 959740 959860 120 0.0
BRD4161A+rs911x (read only) 799456 799576 120 0.0
.text 799448 799568 120 0.0
lock-app BRD4161A+wf200 (read only) 965876 965980 104 0.0
.text 965868 965972 104 0.0
window-app BRD4161A (read only) 909204 909324 120 0.0
.text 909196 909316 120 0.0
esp32 all-clusters-app c3devkit (read only) 1012958 1012992 34 0.0
(read/write) 1482898 1482954 56 0.0
.flash.rodata 213440 213496 56 0.0
.flash.text 1012958 1012992 34 0.0
m5stack (read only) 1067459 1067527 68 0.0
(read/write) 485016 485072 56 0.0
.flash.rodata 243964 244020 56 0.0
.flash.text 1062075 1062143 68 0.0
k32w light k32w061+release (read/write) 658940 658988 48 0.0
.text 582392 582440 48 0.0
lock k32w061+release (read/write) 720916 721020 104 0.0
.text 643920 644024 104 0.0
linux all-clusters-app debug (read only) 2931385 2931545 160 0.0
.rodata 260029 260125 96 0.0
.text 2494914 2494978 64 0.0
all-clusters-minimal-app debug (read only) 2785097 2785257 160 0.0
.rodata 261565 261661 96 0.0
.text 2349266 2349330 64 0.0
bridge-app debug+rpc (read only) 2287665 2287841 176 0.0
.rodata 194848 194944 96 0.0
.text 1931730 1931810 80 0.0
chip-tool debug (read only) 10141773 10141949 176 0.0
.rodata 509301 509397 96 0.0
.text 8226741 8226821 80 0.0
chip-tool-no-interactive-ipv6only arm64 (read only) 9882612 9882852 240 0.0
.rodata 472324 472420 96 0.0
.text 7873860 7874004 144 0.0
lighting-app debug+rpc (read only) 2520433 2520609 176 0.0
.rodata 210632 210728 96 0.0
.text 2140098 2140178 80 0.0
lock-app debug (read only) 2459113 2459273 160 0.0
.rodata 224328 224424 96 0.0
.text 2070482 2070546 64 0.0
ota-provider-app debug (read only) 2296993 2297169 176 0.0
.rodata 200504 200600 96 0.0
.text 1934338 1934418 80 0.0
ota-requestor-app debug (read only) 2412705 2412849 144 0.0
.rodata 204192 204256 64 0.0
.text 2038130 2038210 80 0.0
shell debug (read only) 2604865 2605025 160 0.0
.rodata 230290 230386 96 0.0
.text 2214818 2214882 64 0.0
thermostat-no-ble arm64 (read only) 2571580 2571788 208 0.0
.rodata 163340 163420 80 0.0
.text 2169120 2169248 128 0.0
tv-app debug (read only) 3068849 3069025 176 0.0
.rodata 246176 246272 96 0.0
.text 2635810 2635890 80 0.0
tv-casting-app debug (read only) 5534513 5534689 176 0.0
.rodata 340137 340233 96 0.0
.text 4918146 4918226 80 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read/write) 2438104 2438160 56 0.0
.text 1400748 1400804 56 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1198747 1198835 88 0.0
rodata 155676 155732 56 0.0
text 822552 822596 44 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 (read/write) 1146703 1146807 104 0.0
rodata 132200 132256 56 0.0
text 794772 794816 44 0.0
p6 all-clusters-app default (read/write) 2553928 2554048 120 0.0
.text 1512192 1512312 120 0.0
all-clusters-minimal-app default (read/write) 2499776 2499896 120 0.0
.text 1458040 1458160 120 0.0
light-app default (read/write) 2430520 2430640 120 0.0
.text 1388784 1388904 120 0.0
lock-app default (read/write) 2451216 2451336 120 0.0
.text 1409480 1409600 120 0.0
telink light-switch-app tlsr9518adk80d (read/write) 789088 789176 88 0.0
text 559678 559710 32 0.0
lighting-app tlsr9518adk80d (read/write) 808800 808888 88 0.0
text 576138 576172 34 0.0
Decreases (5 builds for cc13x2_26x2)
platform target config section 48606c6 432255d change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read/write) 191296 191200 -96 -0.1
lock-ftd LP_CC2652R7 (read/write) 150444 150348 -96 -0.1
pump-app LP_CC2652R7 (read/write) 170396 170292 -104 -0.1
pump-controller-app LP_CC2652R7 (read/write) 184616 184520 -96 -0.1
shell LP_CC2652R7 (read/write) 157364 157260 -104 -0.1
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section 48606c6 432255d change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 659567 659663 96 0.0
(read/write) 191296 191200 -96 -0.1
.bss 73756 73756 0 0.0
.data 3356 3356 0 0.0
.rodata 87087 87143 56 0.1
.text 572168 572208 40 0.0
all-clusters-minimal-app LP_CC2652R7 (read only) 648063 648159 96 0.0
(read/write) 157316 157316 0 0.0
.bss 73044 73044 0 0.0
.data 3356 3356 0 0.0
.rodata 90407 90463 56 0.1
.text 557336 557376 40 0.0
lock-ftd LP_CC2652R7 (read only) 690531 690627 96 0.0
(read/write) 150444 150348 -96 -0.1
.bss 70756 70756 0 0.0
.data 3280 3280 0 0.0
.rodata 99163 99219 56 0.1
.text 590884 590924 40 0.0
lock-mtd LP_CC2652R7 (read only) 639931 640035 104 0.0
(read/write) 143888 143888 0 0.0
.bss 66492 66492 0 0.0
.data 3280 3280 0 0.0
.rodata 99043 99099 56 0.1
.text 540396 540444 48 0.0
pump-app LP_CC2652R7 (read only) 671467 671571 104 0.0
(read/write) 170396 170292 -104 -0.1
.bss 70876 70876 0 0.0
.data 3280 3280 0 0.0
.rodata 87259 87315 56 0.1
.text 583724 583772 48 0.0
pump-controller-app LP_CC2652R7 (read only) 657359 657455 96 0.0
(read/write) 184616 184520 -96 -0.1
.bss 70988 70988 0 0.0
.data 3276 3276 0 0.0
.rodata 83103 83159 56 0.1
.text 573776 573816 40 0.0
shell LP_CC2652R7 (read only) 688994 689098 104 0.0
(read/write) 157364 157260 -104 -0.1
.bss 76052 76052 0 0.0
.data 3360 3360 0 0.0
.rodata 109658 109714 56 0.1
.text 579024 579072 48 0.0
cyw30739 light cyw930739m2evb_01 (read/write) 612766 612766 0 0.0
.app_xip_area 469432 469432 0 0.0
.bss 86288 86288 0 0.0
.data 728 728 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
lock cyw930739m2evb_01 (read/write) 610122 610122 0 0.0
.app_xip_area 466612 466612 0 0.0
.bss 86464 86464 0 0.0
.data 732 732 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 616130 616226 96 0.0
.app_xip_area 473684 473780 96 0.0
.bss 85456 85456 0 0.0
.data 672 672 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read only) 924060 924180 120 0.0
(read/write) 132416 132416 0 0.0
.bss 130336 130336 0 0.0
.data 2080 2080 0 0.0
.text 924052 924172 120 0.0
BRD4161A+rpc (read only) 959748 959868 120 0.0
(read/write) 149292 149292 0 0.0
.bss 147008 147008 0 0.0
.data 2284 2284 0 0.0
.text 959740 959860 120 0.0
BRD4161A+rs911x (read only) 799456 799576 120 0.0
(read/write) 128692 128692 0 0.0
.bss 126604 126604 0 0.0
.data 2088 2088 0 0.0
.text 799448 799568 120 0.0
lock-app BRD4161A+wf200 (read only) 965876 965980 104 0.0
(read/write) 129068 129068 0 0.0
.bss 126980 126980 0 0.0
.data 2088 2088 0 0.0
.text 965868 965972 104 0.0
window-app BRD4161A (read only) 909204 909324 120 0.0
(read/write) 132516 132516 0 0.0
.bss 130408 130408 0 0.0
.data 2108 2108 0 0.0
.text 909196 909316 120 0.0
esp32 all-clusters-app c3devkit (read only) 1012958 1012992 34 0.0
(read/write) 1482898 1482954 56 0.0
.dram0.bss 69392 69392 0 0.0
.dram0.data 14632 14632 0 0.0
.flash.rodata 213440 213496 56 0.0
.flash.text 1012958 1012992 34 0.0
.iram0.text 62902 62902 0 0.0
m5stack (read only) 1067459 1067527 68 0.0
(read/write) 485016 485072 56 0.0
.dram0.bss 74912 74912 0 0.0
.dram0.data 34144 34144 0 0.0
.flash.rodata 243964 244020 56 0.0
.flash.text 1062075 1062143 68 0.0
.iram0.text 123267 123267 0 0.0
k32w light k32w061+release (read/write) 658940 658988 48 0.0
.bss 68756 68756 0 0.0
.data 1992 1992 0 0.0
.text 582392 582440 48 0.0
lock k32w061+release (read/write) 720916 721020 104 0.0
.bss 69196 69196 0 0.0
.data 2000 2000 0 0.0
.text 643920 644024 104 0.0
linux all-clusters-app debug (read only) 2931385 2931545 160 0.0
(read/write) 188656 188656 0 0.0
.bss 95744 95744 0 0.0
.data 2048 2048 0 0.0
.data.rel.ro 84664 84664 0 0.0
.dynamic 608 608 0 0.0
.got 4536 4536 0 0.0
.init 27 27 0 0.0
.init_array 1032 1032 0 0.0
.rodata 260029 260125 96 0.0
.text 2494914 2494978 64 0.0
all-clusters-minimal-app debug (read only) 2785097 2785257 160 0.0
(read/write) 180560 180560 0 0.0
.bss 95072 95072 0 0.0
.data 2048 2048 0 0.0
.data.rel.ro 77304 77304 0 0.0
.dynamic 608 608 0 0.0
.got 4488 4488 0 0.0
.init 27 27 0 0.0
.init_array 1032 1032 0 0.0
.rodata 261565 261661 96 0.0
.text 2349266 2349330 64 0.0
bridge-app debug+rpc (read only) 2287665 2287841 176 0.0
(read/write) 159424 159424 0 0.0
.bss 83136 83136 0 0.0
.data 3792 3792 0 0.0
.data.rel.ro 66728 66728 0 0.0
.dynamic 608 608 0 0.0
.got 4392 4392 0 0.0
.init 27 27 0 0.0
.init_array 728 728 0 0.0
.rodata 194848 194944 96 0.0
.text 1931730 1931810 80 0.0
chip-tool debug (read only) 10141773 10141949 176 0.0
(read/write) 609544 609544 0 0.0
.bss 24352 24352 0 0.0
.data 1088 1088 0 0.0
.data.rel.ro 577808 577808 0 0.0
.dynamic 624 624 0 0.0
.got 5008 5008 0 0.0
.init 27 27 0 0.0
.init_array 640 640 0 0.0
.rodata 509301 509397 96 0.0
.text 8226741 8226821 80 0.0
chip-tool-no-interactive-ipv6only arm64 (read only) 9882612 9882852 240 0.0
(read/write) 674225 674225 0 0.0
.bss 42641 42641 0 0.0
.data 1152 1152 0 0.0
.data.rel.ro 613208 613208 0 0.0
.dynamic 528 528 0 0.0
.got 13416 13416 0 0.0
.init 24 24 0 0.0
.init_array 192 192 0 0.0
.rodata 472324 472420 96 0.0
.text 7873860 7874004 144 0.0
lighting-app debug+rpc (read only) 2520433 2520609 176 0.0
(read/write) 163448 163448 0 0.0
.bss 83616 83616 0 0.0
.data 2064 2064 0 0.0
.data.rel.ro 71896 71896 0 0.0
.dynamic 608 608 0 0.0
.got 4424 4424 0 0.0
.init 27 27 0 0.0
.init_array 816 816 0 0.0
.rodata 210632 210728 96 0.0
.text 2140098 2140178 80 0.0
lock-app debug (read only) 2459113 2459273 160 0.0
(read/write) 158096 158096 0 0.0
.bss 82016 82016 0 0.0
.data 1680 1680 0 0.0
.data.rel.ro 68568 68568 0 0.0
.dynamic 608 608 0 0.0
.got 4424 4424 0 0.0
.init 27 27 0 0.0
.init_array 776 776 0 0.0
.rodata 224328 224424 96 0.0
.text 2070482 2070546 64 0.0
ota-provider-app debug (read only) 2296993 2297169 176 0.0
(read/write) 152232 152232 0 0.0
.bss 81696 81696 0 0.0
.data 1912 1912 0 0.0
.data.rel.ro 62840 62840 0 0.0
.dynamic 608 608 0 0.0
.got 4488 4488 0 0.0
.init 27 27 0 0.0
.init_array 672 672 0 0.0
.rodata 200504 200600 96 0.0
.text 1934338 1934418 80 0.0
ota-requestor-app debug (read only) 2412705 2412849 144 0.0
(read/write) 158976 158976 0 0.0
.bss 84000 84000 0 0.0
.data 2200 2200 0 0.0
.data.rel.ro 66936 66936 0 0.0
.dynamic 608 608 0 0.0
.got 4480 4480 0 0.0
.init 27 27 0 0.0
.init_array 728 728 0 0.0
.rodata 204192 204256 64 0.0
.text 2038130 2038210 80 0.0
shell debug (read only) 2604865 2605025 160 0.0
(read/write) 219288 219288 0 0.0
.bss 134504 134504 0 0.0
.data 1232 1232 0 0.0
.data.rel.ro 77808 77808 0 0.0
.dynamic 608 608 0 0.0
.got 4168 4168 0 0.0
.init 27 27 0 0.0
.init_array 936 936 0 0.0
.rodata 230290 230386 96 0.0
.text 2214818 2214882 64 0.0
thermostat-no-ble arm64 (read only) 2571580 2571788 208 0.0
(read/write) 192193 192193 0 0.0
.bss 99489 99489 0 0.0
.data 1688 1688 0 0.0
.data.rel.ro 82928 82928 0 0.0
.dynamic 528 528 0 0.0
.got 5072 5072 0 0.0
.init 24 24 0 0.0
.init_array 400 400 0 0.0
.rodata 163340 163420 80 0.0
.text 2169120 2169248 128 0.0
tv-app debug (read only) 3068849 3069025 176 0.0
(read/write) 289352 289352 0 0.0
.bss 199240 199240 0 0.0
.data 4656 4656 0 0.0
.data.rel.ro 79016 79016 0 0.0
.dynamic 608 608 0 0.0
.got 4840 4840 0 0.0
.init 27 27 0 0.0
.init_array 952 952 0 0.0
.rodata 246176 246272 96 0.0
.text 2635810 2635890 80 0.0
tv-casting-app debug (read only) 5534513 5534689 176 0.0
(read/write) 195664 195664 0 0.0
.bss 84424 84424 0 0.0
.data 2448 2448 0 0.0
.data.rel.ro 102576 102576 0 0.0
.dynamic 608 608 0 0.0
.got 4712 4712 0 0.0
.init 27 27 0 0.0
.init_array 864 864 0 0.0
.rodata 340137 340233 96 0.0
.text 4918146 4918226 80 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2438104 2438160 56 0.0
.bss 208204 208204 0 0.0
.data 5864 5864 0 0.0
.text 1400748 1400804 56 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1198747 1198835 88 0.0
bss 141598 141598 0 0.0
rodata 155676 155732 56 0.0
text 822552 822596 44 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 (read/write) 1146703 1146807 104 0.0
bss 140850 140850 0 0.0
rodata 132200 132256 56 0.0
text 794772 794816 44 0.0
p6 all-clusters-app default (read/write) 2553928 2554048 120 0.0
.bss 143384 143384 0 0.0
.data 2776 2776 0 0.0
.text 1512192 1512312 120 0.0
all-clusters-minimal-app default (read/write) 2499776 2499896 120 0.0
.bss 142664 142664 0 0.0
.data 2776 2776 0 0.0
.text 1458040 1458160 120 0.0
light-app default (read/write) 2430520 2430640 120 0.0
.bss 134744 134744 0 0.0
.data 2592 2592 0 0.0
.text 1388784 1388904 120 0.0
lock-app default (read/write) 2451216 2451336 120 0.0
.bss 134568 134568 0 0.0
.data 2600 2600 0 0.0
.text 1409480 1409600 120 0.0
telink light-switch-app tlsr9518adk80d (read/write) 789088 789176 88 0.0
bss 69892 69892 0 0.0
noinit 40416 40416 0 0.0
text 559678 559710 32 0.0
lighting-app tlsr9518adk80d (read/write) 808800 808888 88 0.0
bss 70140 70140 0 0.0
noinit 40416 40416 0 0.0
text 576138 576172 34 0.0

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this changes the application contract for how exchange refcounts work, it seems quite risky for pre-1.0 work at this point.

In particular, this seems to be assuming OnExchangeClosing implementations throughout which simply don't exist, unless I am missing something, which will lead to dangling pointers and use-after-free...

@bzbarsky-apple
Copy link
Contributor

bzbarsky-apple commented Jun 21, 2022

Verified by unit-tests.

And fwiw, I don't see any tests in this PR covering the interesting scenarios here, and I am quite sure our existing unit tests do not exercise them.

@mrjerryjohns
Copy link
Contributor

I'd tend to agree as well that this is likely something we want post 1.0.

@stale
Copy link

stale bot commented Jul 1, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Jul 1, 2022
@stale
Copy link

stale bot commented Jul 9, 2022

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review - pending stale Stale issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use-after-free when aborting already-closed exchange
3 participants