Skip to content

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Jan 30, 2022

Contribution description

According to ieee802154_radio_confirm_transmit docs, the parameter of
confirm_op for IEEE802154_HAL_OP_TRANSMIT is to be populated as an out
parameter -- but this implementation unconditionally left info
unpopulated. Thus, when run with LLVM, _fsm_state_tx_process_tx_done
looked into an uninitialized info and thus crashed into failing
assertions.


Note that the init was initialized NULL and later that init is checked for being NULL; now there's an actual non-NULL (or maybe non-NULL as the caller may well pass in NULL) writing access.

I checked the wrappers for the other _confirm_op operations, and they either pass in NULL as the argument, or pass in a boolean clear parameter that does get set, so it seems justified to only set info in this particular branch. (One might consider moving the info population step up right away, but it kind of makes sense there if one anticipates there might come to be more ops that use an info in the same way, and as things are the compiler should be free to make that enhancement on its own).

Testing procedure

  • Run the gcoap example with an nrf52 device (I used microbit-v2) and TOOLCHAIN=llvm

Issues/PRs references

Closes: #17591

According to ieee802154_radio_confirm_transmit docs, the parameter of
confirm_op for IEEE802154_HAL_OP_TRANSMIT is to be populated as an out
parameter -- but this implementation unconditionally left info
unpopulated. Thus, when run with LLVM, _fsm_state_tx_process_tx_done
looked into an uninitialized info and thus crashed into failing
assertions.

Closes: RIOT-OS#17591
@chrysn chrysn requested a review from jia200x January 30, 2022 15:19
@chrysn chrysn requested a review from bergzand as a code owner January 30, 2022 15:19
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Jan 30, 2022
@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 30, 2022
@benpicco benpicco added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 30, 2022
@chrysn chrysn merged commit b2b434d into RIOT-OS:master Jan 30, 2022
@chrysn chrysn deleted the fix-17591 branch January 30, 2022 21:03
@chrysn chrysn added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jan 31, 2022
@jia200x
Copy link
Member

jia200x commented Jan 31, 2022

good catch!

@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gcoap example crashes on LLVM
4 participants