Make platform PlatformManager::PostEvent() return status #9573

kpschoedel · 2021-09-09T19:54:10Z

Problem

void PlatformManager::PostEvent() can fail on some platforms, but does
not report this to callers.

In the #9543 case, an intermediate layer unconditionally returned
CHIP_NO_ERROR, and Inet code assumed it could actually test whether
PostEvent() succeeded.

Fixes #9543 PacketBuffer leak

Change overview

Made PlatformManager::PostEvent() return CHIP_ERROR.
Marked it [[nodiscard]] to catch all uses, and made callers
propagate or at least log any error.

Testing

CI.

#### Problem `void PlatformManager::PostEvent()` can fail on some platforms, but does not report this to callers. In the project-chip#9543 case, an intermediate layer unconditionally returned `CHIP_NO_ERROR`, and `Inet` code assumed it could actually test whether `PostEvent()` succeeded. Fixes project-chip#9543 _PacketBuffer leak_ #### Change overview Made `PlatformManager::PostEvent()` return `CHIP_ERROR`. Marked it `[[nodiscard]]` to catch all uses, and made callers propagate or at least log any error. #### Testing CI.

src/include/platform/internal/GenericConfigurationManagerImpl.cpp

src/platform/DeviceControlServer.cpp

src/platform/ESP32/ConnectivityManagerImpl.cpp

src/platform/Linux/ConnectivityManagerImpl.cpp

src/platform/Linux/ThreadStackManagerImpl.cpp

src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp

src/include/platform/PlatformManager.h

…-leak

github-actions · 2021-09-10T18:54:52Z

Size increase report for "gn_qpg-example-build" from 2addf00

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	364	364

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,1590
.debug_loc,0,666
.debug_line,0,412
.text,364,364
.strtab,0,211
.symtab,0,128
.debug_str,0,105
.debug_abbrev,0,87
.debug_frame,0,48
.debug_aranges,0,16
.shstrtab,0,1
.debug_ranges,0,-24
[Unmapped],0,-364

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

github-actions · 2021-09-10T19:30:19Z

Size increase report for "nrfconnect-example-build" from 2addf00

File	Section	File	VM
chip-lock.elf	rodata	184	184
chip-lock.elf	text	148	148
chip-lock.elf	device_handles	-4	-4
chip-shell.elf	text	108	108
chip-shell.elf	rodata	104	100
chip-shell.elf	device_handles	-12	-12

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,1198
.debug_loc,0,535
.debug_line,0,362
.strtab,0,211
rodata,184,184
.symtab,0,160
text,148,148
.debug_abbrev,0,116
.debug_str,0,105
.debug_ranges,0,88
.debug_frame,0,52
.debug_aranges,0,16
.shstrtab,0,1
device_handles,-4,-4

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,1005
.debug_loc,0,547
.debug_line,0,330
.strtab,0,211
.debug_abbrev,0,137
.symtab,0,128
text,108,108
.debug_str,0,105
rodata,100,104
.debug_ranges,0,88
.debug_frame,0,52
.debug_aranges,0,16
.shstrtab,0,1
device_handles,-12,-12

github-actions · 2021-09-10T19:30:42Z

Size increase report for "esp32-example-build" from 2addf00

File	Section	File	VM
chip-temperature-measurement-app.elf	.flash.text	196	196
chip-temperature-measurement-app.elf	.flash.rodata	104	104
chip-shell.elf	.flash.text	115	120
chip-shell.elf	.flash.rodata	56	56
chip-bridge-app.elf	.flash.text	152	152
chip-bridge-app.elf	.flash.rodata	104	104
chip-ipv6only-app.elf	.flash.text	172	172
chip-lock-app.elf	.flash.text	152	152
chip-lock-app.elf	.flash.rodata	104	104
chip-all-clusters-app.elf	.flash.text	200	200
chip-all-clusters-app.elf	.flash.rodata	104	104

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
[Unmapped],0,3796
.debug_info,0,1637
.debug_line,0,840
.debug_loc,0,208
.flash.text,196,196
.shstrtab,0,176
.debug_ranges,0,152
.debug_abbrev,0,111
.debug_str,0,104
.flash.rodata,104,104
.strtab,0,80
[ELF Section Headers],0,80
.xt.prop._ZN4chip11DeviceLayer15PlatformManager14PostEventOrDieEPKNS0_15ChipDeviceEventE,0,60
.symtab,0,48
.debug_frame,0,24
.xt.prop._ZN4chip11DeviceLayer8Internal26GenericPlatformManagerImplINS0_19PlatformManagerImplEE13_ScheduleWorkEPFviEi,0,24
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,12
.debug_aranges,0,8
.xt.lit._ZN4chip11DeviceLayer15PlatformManager14PostEventOrDieEPKNS0_15ChipDeviceEventE,0,8
[1 Others],0,8
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE10_PostEventEPKNS0_15ChipDeviceEventE,0,-12

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
[Unmapped],0,3925
.debug_info,0,1521
.debug_line,0,755
.shstrtab,0,174
.debug_ranges,0,152
.flash.text,120,115
.debug_abbrev,0,111
.debug_str,0,105
.debug_loc,0,100
.strtab,0,90
[ELF Section Headers],0,80
.xt.prop._ZN4chip11DeviceLayer15PlatformManager14PostEventOrDieEPKNS0_15ChipDeviceEventE,0,60
.flash.rodata,56,56
.symtab,0,48
.debug_frame,0,24
.xt.prop._ZN4chip11DeviceLayer8Internal26GenericPlatformManagerImplINS0_19PlatformManagerImplEE13_ScheduleWorkEPFviEi,0,24
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,12
.debug_aranges,0,8
.xt.lit._ZN4chip11DeviceLayer15PlatformManager14PostEventOrDieEPKNS0_15ChipDeviceEventE,0,8
[1 Others],0,8
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE10_PostEventEPKNS0_15ChipDeviceEventE,0,-12

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize
[Unmapped],0,3840
.debug_info,0,1637
.debug_line,0,810
.shstrtab,0,174
.debug_ranges,0,152
.flash.text,152,152
.debug_loc,0,120
.debug_abbrev,0,111
.flash.rodata,104,104
.debug_str,0,102
.strtab,0,90
[ELF Section Headers],0,80
.xt.prop._ZN4chip11DeviceLayer15PlatformManager14PostEventOrDieEPKNS0_15ChipDeviceEventE,0,60
.symtab,0,48
.debug_frame,0,24
.xt.prop._ZN4chip11DeviceLayer8Internal26GenericPlatformManagerImplINS0_19PlatformManagerImplEE13_ScheduleWorkEPFviEi,0,24
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,12
.debug_aranges,0,8
.xt.lit._ZN4chip11DeviceLayer15PlatformManager14PostEventOrDieEPKNS0_15ChipDeviceEventE,0,8
[1 Others],0,8
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE10_PostEventEPKNS0_15ChipDeviceEventE,0,-12

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize
[Unmapped],0,3924
.flash.text,172,172

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
.debug_info,0,1744
.debug_line,0,810
.shstrtab,0,174
.debug_ranges,0,152
.flash.text,152,152
.debug_abbrev,0,111
.debug_str,0,106
.flash.rodata,104,104
.strtab,0,90
[ELF Section Headers],0,80
.xt.prop._ZN4chip11DeviceLayer15PlatformManager14PostEventOrDieEPKNS0_15ChipDeviceEventE,0,60
.symtab,0,48
.debug_frame,0,24
.xt.prop._ZN4chip11DeviceLayer8Internal26GenericPlatformManagerImplINS0_19PlatformManagerImplEE13_ScheduleWorkEPFviEi,0,24
.debug_loc,0,13
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,12
.debug_aranges,0,8
.xt.lit._ZN4chip11DeviceLayer15PlatformManager14PostEventOrDieEPKNS0_15ChipDeviceEventE,0,8
[1 Others],0,8
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE10_PostEventEPKNS0_15ChipDeviceEventE,0,-12
[Unmapped],0,-256

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,1347
.debug_line,0,677
.debug_loc,0,356
.flash.text,200,200
.debug_str,0,105
.debug_ranges,0,104
.flash.rodata,104,104
.strtab,0,90
.debug_abbrev,0,89
.debug_frame,0,32
.symtab,0,16
.debug_aranges,0,8
.riscv.attributes,0,2
.shstrtab,0,2
[Unmapped],0,-304

woody-apple · 2021-09-13T18:51:58Z

@kpschoedel ?

andy31415 · 2021-09-14T16:00:05Z

@kpschoedel ?

Kevin is OOO for another 2 weeks. @mrjerryjohns - should your comments be addressed by code changes in this PR or are you ok to merge and change as a followup to expedite changes going in?

mrjerryjohns

With the latest changes to now clearly provide a method that dies if it fails, vs a method that doesn't, Kevin's addressed my concerns.

boring-cyborg bot added darwin efr32 esp32 k32w linux openthread platform zephyr labels Sep 9, 2021

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, saurabhst, vivien-apple and woody-apple September 9, 2021 19:55

pullapprove bot added the review - pending label Sep 9, 2021

andy31415 approved these changes Sep 9, 2021

View reviewed changes

saurabhst approved these changes Sep 9, 2021

View reviewed changes

bzbarsky-apple requested changes Sep 9, 2021

View reviewed changes

kpschoedel mentioned this pull request Sep 9, 2021

[ESP32] ConnectivityManagerImpl — should UpdateInternetConnectivityState() happen if PostEvent() fails? #9577

Open

address review comments

2a27822

restyled-io bot mentioned this pull request Sep 9, 2021

Restyle Make platform PlatformManager::PostEvent() return status #9578

Closed

restyle

aaa4cf8

mrjerryjohns reviewed Sep 10, 2021

View reviewed changes

src/include/platform/PlatformManager.h Outdated Show resolved Hide resolved

src/include/platform/PlatformManager.h Show resolved Hide resolved

woody-apple added the SDK Approved label Sep 10, 2021

kpschoedel added 2 commits September 10, 2021 14:28

Merge remote-tracking branch 'origin/master' into x9543-packet-buffer…

8a1c458

…-leak

replace PostEventLoggingErrors with PostEventOrDie

247ef48

fix P6

5a752dc

bzbarsky-apple approved these changes Sep 11, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Sep 11, 2021

mrjerryjohns self-requested a review September 14, 2021 17:20

mrjerryjohns approved these changes Sep 14, 2021

View reviewed changes

woody-apple merged commit 6fd3811 into project-chip:master Sep 14, 2021

kpschoedel deleted the x9543-packet-buffer-leak branch October 4, 2021 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make platform PlatformManager::PostEvent() return status #9573

Make platform PlatformManager::PostEvent() return status #9573

kpschoedel commented Sep 9, 2021

github-actions bot commented Sep 10, 2021

github-actions bot commented Sep 10, 2021

github-actions bot commented Sep 10, 2021

woody-apple commented Sep 13, 2021

andy31415 commented Sep 14, 2021

mrjerryjohns left a comment

Make platform PlatformManager::PostEvent() return status #9573

Make platform PlatformManager::PostEvent() return status #9573

Conversation

kpschoedel commented Sep 9, 2021

Problem

Change overview

Testing

github-actions bot commented Sep 10, 2021

github-actions bot commented Sep 10, 2021

github-actions bot commented Sep 10, 2021

woody-apple commented Sep 13, 2021

andy31415 commented Sep 14, 2021

mrjerryjohns left a comment

Choose a reason for hiding this comment