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.

Sign up for GitHub

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

Jump to bottom

Mark MCSP responses as NOT_IMPLEMENTED #14853

Merged
merged 3 commits into from
Feb 8, 2022
Merged

Mark MCSP responses as NOT_IMPLEMENTED #14853

merged 3 commits into from
Feb 8, 2022

Conversation

andy31415
Copy link
Contributor

@andy31415 andy31415 commented Feb 7, 2022 •
edited
Loading

Problem

Potential crash in AsSecureSession call.

Change overview

Mark MCSP as not implemented. When implementing it should use Group sessions instead of secure sessions.

Fixes #13668

Testing

  • flashed and commissioned a devkit-c-all-clusters (to ensure this code is not used during regular secure seession support)
  • checked out ae05dfc and applied these patches on top, no crash

@bzbarsky-apple
Copy link
Contributor

however the change seems trivial enough and can be validated by CI.

We don't have any CI coverage for this code.

@andy31415
Copy link
Contributor Author

however the change seems trivial enough and can be validated by CI.

We don't have any CI coverage for this code.

I guess it does not make it worse then (and validates compilation)? I did a CP on the old code and it seemed to avoid the crash - which is expected because it validates that the cast is ok.

@andy31415 andy31415 changed the title Validate session type before trying to cast and return the secure session counter Mark MCSP responses as NOT_IMPLEMENTED Feb 7, 2022
@github-actions
Copy link

github-actions bot commented Feb 7, 2022 •
edited
Loading

PR #14853: Size comparison from a92d1a8 to 8eb9f50

Increases above 0.2%:

platform target config section a92d1a8 8eb9f50 change % change
nrfconnect lighting-app nrf52840dk_nrf52840 rodata 117760 118048 288 0.2
nrf52840dk_nrf52840+rpc rodata 109220 109508 288 0.3
nrf52840dongle_nrf52840 rodata 116648 116936 288 0.2
nrf5340dk_nrf5340_cpuapp rodata 111016 111304 288 0.3
lock-app nrf52840dk_nrf52840 rodata 106160 106448 288 0.3
nrf5340dk_nrf5340_cpuapp rodata 99332 99620 288 0.3
pump-controller-app nrf52840dk_nrf52840 rodata 105636 105924 288 0.3
Increases (26 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section a92d1a8 8eb9f50 change % change
cyw30739 light cyw930739m2evb_01 (read/write) 593918 594178 260 0.0
.app_xip_area 499772 500032 260 0.1
lock cyw930739m2evb_01 (read/write) 551846 552106 260 0.0
.app_xip_area 459260 459520 260 0.1
efr32 lighting-app BRD4161A (read only) 865236 865524 288 0.0
.text 865228 865516 288 0.0
BRD4161A+rpc (read only) 852692 852996 304 0.0
.text 852684 852988 304 0.0
window-app BRD4161A (read only) 837472 837760 288 0.0
.text 837464 837752 288 0.0
esp32 all-clusters-app c3devkit (read only) 937038 937070 32 0.0
(read/write) 1401242 1401538 296 0.0
.flash.rodata 198000 198296 296 0.1
.flash.text 937038 937070 32 0.0
m5stack (read/write) 465592 465880 288 0.1
.flash.rodata 224608 224896 288 0.1
k32w lock k32w061+release (read/write) 679948 680220 272 0.0
.text 595328 595600 272 0.0
linux chip-tool-ipv6only arm64 (read only) 7239588 7241060 1472 0.0
.text 6241972 6243444 1472 0.0
thermostat-no-ble arm64 (read only) 2127748 2129556 1808 0.1
.rodata 131412 131700 288 0.2
.text 1778384 1779904 1520 0.1
mbed all-clusters-app CY8CPROTO_062_4343W+release (read/write) 2389200 2389488 288 0.0
.text 1351800 1352088 288 0.0
lighting-app CY8CPROTO_062_4343W+release (read/write) 2348464 2348688 224 0.0
.text 1311064 1311288 224 0.0
lock-app CY8CPROTO_062_4343W+release (read/write) 2312384 2312672 288 0.0
.text 1274984 1275272 288 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 1004243 1004499 256 0.0
rodata 117760 118048 288 0.2
nrf52840dk_nrf52840+rpc (read/write) 975735 975991 256 0.0
rodata 109220 109508 288 0.3
nrf52840dongle_nrf52840 (read/write) 1021063 1021319 256 0.0
rodata 116648 116936 288 0.2
nrf5340dk_nrf5340_cpuapp (read/write) 910902 911174 272 0.0
rodata 111016 111304 288 0.3
lock-app nrf52840dk_nrf52840 (read/write) 936515 936771 256 0.0
rodata 106160 106448 288 0.3
nrf5340dk_nrf5340_cpuapp (read/write) 844010 844266 256 0.0
rodata 99332 99620 288 0.3
pump-controller-app nrf52840dk_nrf52840 (read/write) 934087 934343 256 0.0
rodata 105636 105924 288 0.3
p6 all-clusters-app default (read/write) 2450352 2450640 288 0.0
.text 1408616 1408904 288 0.0
light-app default (read/write) 2353784 2354072 288 0.0
.text 1312048 1312336 288 0.0
lock-app default (read/write) 2318984 2319272 288 0.0
.text 1277248 1277536 288 0.0
qpg lighting-app qpg6105+debug (read only) 585492 585756 264 0.0
.text 580172 580436 264 0.0
lock-app qpg6105+debug (read only) 531568 531824 256 0.0
.text 526248 526504 256 0.0
telink lighting-app tlsr9518adk80d (read/write) 865726 866054 328 0.0
text 607342 607374 32 0.0
Decreases (13 builds for cyw30739, esp32, k32w, mbed, nrfconnect)
platform target config section a92d1a8 8eb9f50 change % change
cyw30739 ota-requestor cyw930739m2evb_01 (read/write) 576290 576150 -140 -0.0
.app_xip_area 474384 474244 -140 -0.0
esp32 all-clusters-app m5stack (read only) 985011 984987 -24 -0.0
.flash.text 979627 979603 -24 -0.0
k32w light k32w061+release (read/write) 678480 678336 -144 -0.0
.text 594228 594084 -144 -0.0
mbed shell CY8CPROTO_062_4343W+release (read/write) 2302836 2302644 -192 -0.0
.text 1265408 1265216 -192 -0.0
nrfconnect lighting-app nrf52840dk_nrf52840 text 686260 686232 -28 -0.0
nrf52840dk_nrf52840+rpc text 670912 670880 -32 -0.0
nrf52840dongle_nrf52840 text 691832 691800 -32 -0.0
nrf5340dk_nrf5340_cpuapp text 601284 601256 -28 -0.0
lock-app nrf52840dk_nrf52840 text 632196 632168 -28 -0.0
nrf5340dk_nrf5340_cpuapp text 547996 547964 -32 -0.0
pump-app nrf52840dk_nrf52840 (read/write) 939131 939003 -128 -0.0
text 634536 634396 -140 -0.0
pump-controller-app nrf52840dk_nrf52840 text 630476 630444 -32 -0.0
shell nrf52840dk_nrf52840 (read/write) 803327 803135 -192 -0.0
text 535988 535808 -180 -0.0
Full report (34 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section a92d1a8 8eb9f50 change % change
cyw30739 light cyw930739m2evb_01 (read/write) 593918 594178 260 0.0
.app_xip_area 499772 500032 260 0.1
.bss 76868 76868 0 0.0
.data 624 624 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
lock cyw930739m2evb_01 (read/write) 551846 552106 260 0.0
.app_xip_area 459260 459520 260 0.1
.bss 75340 75340 0 0.0
.data 588 588 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
ota-requestor cyw930739m2evb_01 (read/write) 576290 576150 -140 -0.0
.app_xip_area 474384 474244 -140 -0.0
.bss 84332 84332 0 0.0
.data 532 532 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read only) 865236 865524 288 0.0
(read/write) 126620 126620 0 0.0
.bss 124696 124696 0 0.0
.data 1924 1924 0 0.0
.text 865228 865516 288 0.0
BRD4161A+rpc (read only) 852692 852996 304 0.0
(read/write) 143280 143280 0 0.0
.bss 141256 141256 0 0.0
.data 2024 2024 0 0.0
.text 852684 852988 304 0.0
window-app BRD4161A (read only) 837472 837760 288 0.0
(read/write) 125264 125264 0 0.0
.bss 123384 123384 0 0.0
.data 1880 1880 0 0.0
.text 837464 837752 288 0.0
esp32 all-clusters-app c3devkit (read only) 937038 937070 32 0.0
(read/write) 1401242 1401538 296 0.0
.dram0.bss 70064 70064 0 0.0
.dram0.data 14276 14276 0 0.0
.flash.rodata 198000 198296 296 0.1
.flash.text 937038 937070 32 0.0
.iram0.text 62056 62056 0 0.0
m5stack (read only) 985011 984987 -24 -0.0
(read/write) 465592 465880 288 0.1
.dram0.bss 74816 74816 0 0.0
.dram0.data 34040 34040 0 0.0
.flash.rodata 224608 224896 288 0.1
.flash.text 979627 979603 -24 -0.0
.iram0.text 123399 123399 0 0.0
k32w light k32w061+release (read/write) 678480 678336 -144 -0.0
.bss 76568 76568 0 0.0
.data 1884 1884 0 0.0
.text 594228 594084 -144 -0.0
lock k32w061+release (read/write) 679948 680220 272 0.0
.bss 76896 76896 0 0.0
.data 1924 1924 0 0.0
.text 595328 595600 272 0.0
linux chip-tool-ipv6only arm64 (read only) 7239588 7241060 1472 0.0
(read/write) 287921 287921 0 0.0
.bss 50641 50641 0 0.0
.data 1176 1176 0 0.0
.data.rel.ro 186528 186528 0 0.0
.dynamic 560 560 0 0.0
.got 45784 45784 0 0.0
.init 24 24 0 0.0
.init_array 192 192 0 0.0
.rodata 392780 392780 0 0.0
.text 6241972 6243444 1472 0.0
thermostat-no-ble arm64 (read only) 2127748 2129556 1808 0.1
(read/write) 140561 140561 0 0.0
.bss 57601 57601 0 0.0
.data 968 968 0 0.0
.data.rel.ro 74872 74872 0 0.0
.dynamic 560 560 0 0.0
.got 4152 4152 0 0.0
.init 24 24 0 0.0
.init_array 328 328 0 0.0
.rodata 131412 131700 288 0.2
.text 1778384 1779904 1520 0.1
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2389200 2389488 288 0.0
.bss 188972 188972 0 0.0
.data 5296 5296 0 0.0
.text 1351800 1352088 288 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2348464 2348688 224 0.0
.bss 180936 180936 0 0.0
.data 5600 5600 0 0.0
.text 1311064 1311288 224 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2312384 2312672 288 0.0
.bss 180824 180824 0 0.0
.data 5584 5584 0 0.0
.text 1274984 1275272 288 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1139712 1139712 0 0.0
.bss 11756 11756 0 0.0
.data 4368 4368 0 0.0
.text 103096 103096 0 0.0
shell CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2302836 2302644 -192 -0.0
.bss 178100 178100 0 0.0
.data 5400 5400 0 0.0
.text 1265408 1265216 -192 -0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 1004243 1004499 256 0.0
bss 121280 121280 0 0.0
rodata 117760 118048 288 0.2
text 686260 686232 -28 -0.0
nrf52840dk_nrf52840+rpc (read/write) 975735 975991 256 0.0
bss 117136 117136 0 0.0
rodata 109220 109508 288 0.3
text 670912 670880 -32 -0.0
nrf52840dongle_nrf52840 (read/write) 1021063 1021319 256 0.0
bss 122644 122644 0 0.0
rodata 116648 116936 288 0.2
text 691832 691800 -32 -0.0
nrf5340dk_nrf5340_cpuapp (read/write) 910902 911174 272 0.0
bss 117840 117840 0 0.0
rodata 111016 111304 288 0.3
text 601284 601256 -28 -0.0
lock-app nrf52840dk_nrf52840 (read/write) 936515 936771 256 0.0
bss 119616 119616 0 0.0
rodata 106160 106448 288 0.3
text 632196 632168 -28 -0.0
nrf5340dk_nrf5340_cpuapp (read/write) 844010 844266 256 0.0
bss 116212 116212 0 0.0
rodata 99332 99620 288 0.3
text 547996 547964 -32 -0.0
pigweed-app nrf52840dk_nrf52840 (read/write) 541779 541779 0 0.0
bss 52588 52588 0 0.0
rodata 50048 50048 0 0.0
text 376940 376940 0 0.0
pump-app nrf52840dk_nrf52840 (read/write) 939131 939003 -128 -0.0
bss 119368 119368 0 0.0
rodata 106632 106632 0 0.0
text 634536 634396 -140 -0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 934087 934343 256 0.0
bss 119372 119372 0 0.0
rodata 105636 105924 288 0.3
text 630476 630444 -32 -0.0
shell nrf52840dk_nrf52840 (read/write) 803327 803135 -192 -0.0
bss 111232 111232 0 0.0
rodata 78532 78532 0 0.0
text 535988 535808 -180 -0.0
p6 all-clusters-app default (read/write) 2450352 2450640 288 0.0
.bss 117128 117128 0 0.0
.data 2584 2584 0 0.0
.text 1408616 1408904 288 0.0
light-app default (read/write) 2353784 2354072 288 0.0
.bss 106128 106128 0 0.0
.data 2432 2432 0 0.0
.text 1312048 1312336 288 0.0
lock-app default (read/write) 2318984 2319272 288 0.0
.bss 105848 105848 0 0.0
.data 2392 2392 0 0.0
.text 1277248 1277536 288 0.0
qpg lighting-app qpg6105+debug (read only) 585492 585756 264 0.0
(read/write) 146940 146940 0 0.0
.bss 88856 88856 0 0.0
.data 1088 1088 0 0.0
.text 580172 580436 264 0.0
lock-app qpg6105+debug (read only) 531568 531824 256 0.0
(read/write) 146940 146940 0 0.0
.bss 88304 88304 0 0.0
.data 1024 1024 0 0.0
.text 526248 526504 256 0.0
persistent-storage-app qpg6105+debug (read only) 99548 99548 0 0.0
(read/write) 146940 146940 0 0.0
.bss 24004 24004 0 0.0
.data 176 176 0 0.0
.text 94228 94228 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 865726 866054 328 0.0
bss 89208 89208 0 0.0
noinit 37160 37160 0 0.0
text 607342 607374 32 0.0

@woody-apple woody-apple linked an issue Feb 8, 2022 that may be closed by this pull request
Add MCSP to GroupMessageDispatch #11689
Closed
@woody-apple
Copy link
Contributor

Fast tracking given this is removing code.

@woody-apple woody-apple merged commit 2ffbe9f into project-chip:master Feb 8, 2022
@markus-becker-tridonic-com markus-becker-tridonic-com mentioned this pull request Nov 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@anush-apple anush-apple Awaiting requested review from anush-apple

@balducci-apple balducci-apple Awaiting requested review from balducci-apple

@Byungjoo-Lee Byungjoo-Lee Awaiting requested review from Byungjoo-Lee

@carol-apple carol-apple Awaiting requested review from carol-apple

@cecille cecille Awaiting requested review from cecille

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chshu chshu Awaiting requested review from chshu

@chulspro chulspro Awaiting requested review from chulspro

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@dhrishi dhrishi Awaiting requested review from dhrishi

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@emargolis emargolis Awaiting requested review from emargolis

@franck-apple franck-apple Awaiting requested review from franck-apple

@gjc13 gjc13 Awaiting requested review from gjc13

@harimau-qirex harimau-qirex Awaiting requested review from harimau-qirex

@hawk248 hawk248 Awaiting requested review from hawk248

@holbrookt holbrookt Awaiting requested review from holbrookt

@harsha-rajendran harsha-rajendran Awaiting requested review from harsha-rajendran

@isiu-apple isiu-apple Awaiting requested review from isiu-apple

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@kghost kghost Awaiting requested review from kghost

@LuDuda LuDuda Awaiting requested review from LuDuda

@lzgrablic02 lzgrablic02 Awaiting requested review from lzgrablic02

@mrjerryjohns mrjerryjohns Awaiting requested review from mrjerryjohns

@msandstedt msandstedt Awaiting requested review from msandstedt

@mspang mspang Awaiting requested review from mspang

@robszewczyk robszewczyk Awaiting requested review from robszewczyk

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@selissia selissia Awaiting requested review from selissia

@tecimovic tecimovic Awaiting requested review from tecimovic

@vijs vijs Awaiting requested review from vijs

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@wbschiller wbschiller Awaiting requested review from wbschiller

@xylophone21 xylophone21 Awaiting requested review from xylophone21

@yufengwangca yufengwangca Awaiting requested review from yufengwangca

@woody-apple woody-apple Awaiting requested review from woody-apple

Assignees
No one assigned
Labels
fast track review - pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash doing message counter sync Add MCSP to GroupMessageDispatch
4 participants
@andy31415 @bzbarsky-apple @woody-apple @msandstedt