Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp #25485

vivien-apple · 2023-03-06T10:30:07Z

Problem

SystemStats.cpp accesses some static variables but does not check that the access is from the chip thread. The ble darwin implementation does access those from the ble work queue because it tries to create a PacketBufferHandle.
This PR ensure that those are accesses onto the chip work queue, and that SystemStats.cpp will complains if that happens again.

github-actions · 2023-03-06T11:00:52Z

PR #25485: Size comparison from 32fb896 to 5a1ec7e

Increases (1 build for cc32xx)

platform	target	config	section	`32fb896`	`5a1ec7e`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	20262927	20262928	1	0.0
			.debug_line	`2657822`	2657826	4	0.0

Full report (1 build for cc32xx)

platform	target	config	section	`32fb896`	`5a1ec7e`	change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0
			(read only)	643465	643465	0
			(read/write)	203688	203688	0
			.ARM.attributes	44	44	0
			.ARM.exidx	8	8	0
			.bss	197088	197088	0
			.comment	194	194	0
			.data	1480	1480	0
			.debug_abbrev	930213	930213	0
			.debug_aranges	87336	87336	0
			.debug_frame	300024	300024	0
			.debug_info	20262927	20262928	1
			.debug_line	`2657822`	2657826	4
			.debug_loc	2800026	2800026	0
			.debug_ranges	282240	282240	0
			.debug_str	3023883	3023883	0
			.ramVecs	780	780	0
			.resetVecs	64	64	0
			.rodata	105929	105929	0
			.shstrtab	232	232	0
			.stab	204	204	0
			.stabstr	441	441	0
			.stack	2048	2048	0
			.strtab	378514	378514	0
			.symtab	256560	256560	0
			.text	535412	535412	0

vivien-apple · 2023-03-06T12:48:35Z

This issue is somehow similar to #25313 where a static is used from multiple threads/queues.

github-actions · 2023-03-06T13:05:21Z

PR #25485: Size comparison from 32fb896 to 3dc49d7

Increases (1 build for cc32xx)

platform	target	config	section	`32fb896`	`3dc49d7`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	20262927	20262928	1	0.0
			.debug_line	`2657822`	2657826	4	0.0

Full report (2 builds for cc32xx, mbed)

platform	target	config	section	`32fb896`	`3dc49d7`	change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0
			(read only)	643465	643465	0
			(read/write)	203688	203688	0
			.ARM.attributes	44	44	0
			.ARM.exidx	8	8	0
			.bss	197088	197088	0
			.comment	194	194	0
			.data	1480	1480	0
			.debug_abbrev	930213	930213	0
			.debug_aranges	87336	87336	0
			.debug_frame	300024	300024	0
			.debug_info	20262927	20262928	1
			.debug_line	`2657822`	2657826	4
			.debug_loc	2800026	2800026	0
			.debug_ranges	282240	282240	0
			.debug_str	3023883	3023883	0
			.ramVecs	780	780	0
			.resetVecs	64	64	0
			.rodata	105929	105929	0
			.shstrtab	232	232	0
			.stab	204	204	0
			.stabstr	441	441	0
			.stack	2048	2048	0
			.strtab	378514	378514	0
			.symtab	256560	256560	0
			.text	535412	535412	0
mbed	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0
			(read/write)	`2467384`	`2467384`	0
			.bss	215804	215804	0
			.data	5880	5880	0
			.text	1430028	1430028	0

vivien-apple · 2023-03-06T14:37:31Z

For reference I found that while working on #25374.

The Tsan stack was:

WARNING: ThreadSanitizer: data race (pid=1386)
  Write of size 1 at 0x00010bd57fc9 by thread T3:
    #0 chip::System::PacketBufferHandle::New(unsigned long, unsigned short) SystemPacketBuffer.cpp:503 (chip-tool:x86_64+0x100dd0183)
    #1 chip::System::PacketBufferHandle::NewWithData(void const*, unsigned long, unsigned short, unsigned short) SystemPacketBuffer.cpp:536 (chip-tool:x86_64+0x100dd0536)
    #2 -[BleConnection peripheral:didUpdateValueForCharacteristic:error:] BleConnectionDelegateImpl.mm:476 (chip-tool:x86_64+0x100fa0fea)
    #3 -[CBPeripheral handleAttributeEvent:args:attributeSelector:delegateSelector:delegateFlag:] <null>:2 (CoreBluetooth:x86_64+0x346a9)
    #4 _dispatch_client_callout <null>:2 (libdispatch.dylib:x86_64+0x2a43)

  Previous write of size 1 at 0x00010bd57fc9 by thread T2:
    #0 chip::System::PacketBuffer::Free(chip::System::PacketBuffer*) SystemPacketBuffer.cpp:578 (chip-tool:x86_64+0x100dcebe4)
    #1 chip::System::PacketBufferHandle::operator=(std::nullptr_t) SystemPacketBuffer.h:451 (chip-tool:x86_64+0x1000f12d8)
    #2 chip::Ble::BLEEndPoint::HandleFragmentConfirmationReceived() BLEEndPoint.cpp:881 (chip-tool:x86_64+0x100dabf7e)
    #3 chip::Ble::BLEEndPoint::HandleGattSendConfirmationReceived() BLEEndPoint.cpp:928 (chip-tool:x86_64+0x100dac23e)
    #4 chip::Ble::BleLayer::HandleAckReceived(void*) BleLayer.cpp:611 (chip-tool:x86_64+0x100db14d1)
    #5 chip::Ble::BleLayer::HandleWriteConfirmation(void*, chip::Ble::ChipBleUUID const*, chip::Ble::ChipBleUUID const*) BleLayer.cpp:575 (chip-tool:x86_64+0x100db1376)
    #6 __65-[BleConnection peripheral:didWriteValueForCharacteristic:error:]_block_invoke BleConnectionDelegateImpl.mm:421 (chip-tool:x86_64+0x100fa00d3)
    #7 __tsan::invoke_and_release_block(void*) <null>:3 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x7f44b)
    #8 _dispatch_client_callout <null>:2 (libdispatch.dylib:x86_64+0x2a43)

src/platform/Darwin/BleConnectionDelegateImpl.mm

src/setup_payload/tests/BUILD.gn

src/lib/core/tests/BUILD.gn

…c/system/SystemStats.cpp

…does not creates a PacketBufferHandle from the ble work queue but from the chip work queue

github-actions · 2023-03-07T11:27:34Z

PR #25485: Size comparison from fc0e4a2 to 91a04fb

Increases (1 build for cc32xx)

platform	target	config	section	`fc0e4a2`	`91a04fb`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_line	2659698	2659702	4	0.0

Decreases (1 build for cc32xx)

platform	target	config	section	`fc0e4a2`	`91a04fb`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	`2026706`	20267066	-1	-0.0

Full report (1 build for cc32xx)

platform	target	config	section	`fc0e4a2`	`91a04fb`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0	0.0
			(read only)	644425	644425	0	0.0
			(read/write)	203688	203688	0	0.0
			.ARM.attributes	44	44	0	0.0
			.ARM.exidx	8	8	0	0.0
			.bss	197088	197088	0	0.0
			.comment	194	194	0	0.0
			.data	1480	1480	0	0.0
			.debug_abbrev	930235	930235	0	0.0
			.debug_aranges	87336	87336	0	0.0
			.debug_frame	300028	300028	0	0.0
			.debug_info	`2026706`	20267066	-1	-0.0
			.debug_line	2659698	2659702	4	0.0
			.debug_loc	2802749	2802749	0	0.0
			.debug_ranges	282952	282952	0	0.0
			.debug_str	3023892	3023892	0	0.0
			.ramVecs	780	780	0	0.0
			.resetVecs	64	64	0	0.0
			.rodata	105929	105929	0	0.0
			.shstrtab	232	232	0	0.0
			.stab	204	204	0	0.0
			.stabstr	441	441	0	0.0
			.stack	2048	2048	0	0.0
			.strtab	378514	378514	0	0.0
			.symtab	256624	256624	0	0.0
			.text	536372	536372	0	0.0

…cs in src/system/SystemStats.cpp (#25485)" This reverts commit 8a12695.

…c/system/SystemStats.cpp (project-chip#25485) * Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp * Update src/platform/Darwin/BleConnectionDelegateImpl.mm such that it does not creates a PacketBufferHandle from the ble work queue but from the chip work queue

vivien-apple self-assigned this Mar 6, 2023

github-actions bot added platform system labels Mar 6, 2023

pullapprove bot added the review - pending label Mar 6, 2023

vivien-apple changed the title ~~System stats chip thread only~~ Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp Mar 6, 2023

vivien-apple force-pushed the SystemStats_ChipThreadOnly branch from 5a1ec7e to 3dc49d7 Compare March 6, 2023 12:47

github-actions bot added core lib setup payload labels Mar 6, 2023

Damian-Nordic approved these changes Mar 6, 2023

View reviewed changes

bzbarsky-apple requested changes Mar 6, 2023

View reviewed changes

src/platform/Darwin/BleConnectionDelegateImpl.mm Outdated Show resolved Hide resolved

src/setup_payload/tests/BUILD.gn Show resolved Hide resolved

src/lib/core/tests/BUILD.gn Show resolved Hide resolved

vivien-apple added 2 commits March 7, 2023 12:14

Use assertChipStackLockedByCurrentThread when accessing statics in sr…

16d4596

…c/system/SystemStats.cpp

Update src/platform/Darwin/BleConnectionDelegateImpl.mm such that it …

91a04fb

…does not creates a PacketBufferHandle from the ble work queue but from the chip work queue

vivien-apple requested a review from bzbarsky-apple March 7, 2023 11:15

vivien-apple force-pushed the SystemStats_ChipThreadOnly branch from 3dc49d7 to 91a04fb Compare March 7, 2023 11:15

pullapprove bot requested review from joonhaengHeo and younghak-hwang March 8, 2023 12:04

bzbarsky-apple approved these changes Mar 8, 2023

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Mar 8, 2023

bzbarsky-apple merged commit 8a12695 into project-chip:master Mar 8, 2023

wy-hh mentioned this pull request Mar 10, 2023

[BUG] Chip Stack Protection in LwIP UDP receive path #25604

Closed

tcarmelveilleux added a commit that referenced this pull request Mar 11, 2023

Revert "Use assertChipStackLockedByCurrentThread when accessing stati…

e238867

…cs in src/system/SystemStats.cpp (#25485)" This reverts commit 8a12695.

tcarmelveilleux mentioned this pull request Mar 11, 2023

Revert "Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp" #25638

Closed

jmartinez-silabs mentioned this pull request Mar 13, 2023

Remove asserts for Chip Task lock in SystemStats.cpp #25656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp #25485

Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp #25485

vivien-apple commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

vivien-apple commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

vivien-apple commented Mar 6, 2023

github-actions bot commented Mar 7, 2023

Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp #25485

Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp #25485

Conversation

vivien-apple commented Mar 6, 2023

Problem

github-actions bot commented Mar 6, 2023

vivien-apple commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

vivien-apple commented Mar 6, 2023

github-actions bot commented Mar 7, 2023