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

samples: bluetooth: central_iso: unchecked buffer allocation #72699

Open
msmttchr opened this issue May 13, 2024 · 3 comments
Open

samples: bluetooth: central_iso: unchecked buffer allocation #72699

msmttchr opened this issue May 13, 2024 · 3 comments
Assignees
Labels
area: Bluetooth ISO Bluetooth LE Isochronous Channels area: Bluetooth area: Samples Samples bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@msmttchr
Copy link
Contributor

msmttchr commented May 13, 2024

Describe the bug
In central_iso sample (function iso_timer_timeout), no check on buffer allocation failure is performed.
See code snippet:

buf = net_buf_alloc(&tx_pool, K_FOREVER);
net_buf_reserve(buf, BT_ISO_CHAN_SEND_RESERVE);
net_buf_add_mem(buf, buf_data, len_to_send);
ret = bt_iso_chan_send(&iso_chan, buf, seq_num++);

As a consequence, a system crash is observed when buffer allocation fails.

To Reproduce
Not so easy to reproduce and it depends on the buffer allocation policy.
Error observed while running a new platform against Nordic nRF52840 board.

Expected behavior
Sample should be robust against buffer allocation failure, e.g. throwing an error or a warning an retrying.

Impact
Annoyance, especially when validating new platform prone to have genuine bugs.

Logs and console output
Zephyr crash log.

Environment (please complete the following information):

  • OS: Windows
  • Toolchain Zephyr SDK

Additional context
Actually, from quick check, having K_FOREVER as timeout should avoid to get NULL as return value, but indeed this observed.

@msmttchr msmttchr added the bug The issue is a bug, or the PR is fixing a bug label May 13, 2024
@cvinayak
Copy link
Contributor

Please provide details on how to build and test in the To Reproduce section, and also mention the commit SHA used to build.

Actually, from quick check, having K_FOREVER as timeout should avoid to get NULL as return value, but indeed this observed.

Lowering the priority of the issue, as steps to reproduce is unclear.

@cvinayak cvinayak added area: Bluetooth area: Samples Samples area: Bluetooth ISO Bluetooth LE Isochronous Channels priority: low Low impact/importance bug labels May 13, 2024
@msmttchr
Copy link
Contributor Author

It looks like related/introduced to/by #71697

@jori-nordic
Copy link
Contributor

Yep. A bunch of (mostly audio or iso-related) samples do this.

Samples should've been programmed better, and not block the whole syswq by allocating with K_FOREVER in a callback.
I was not familiar enough with them to propose a patch when I did #71697 . Maybe someone else is?

Note that getting NULL was probably possible before:
https://github.com/zephyrproject-rtos/zephyr/pull/71697/files#diff-7d20cda516d057def150f03b4d2cdce79dc768dbc8ec7c3fab9d463265572466L1322-L1325

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth ISO Bluetooth LE Isochronous Channels area: Bluetooth area: Samples Samples bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

No branches or pull requests

3 participants