Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The guard fixes the undefined shift while preserving behavior for smaller channel masks.
Review effort: Balanced
Findings: None
What changed in this PR
Prevents undefined 32-bit shifts when generating eight-channel ALH channel maps.
Changes:
- Skips absent-channel filling when all eight channels are present.
- Documents the avoided undefined behavior.
| File | Description |
|---|---|
src/audio/copier/copier_dai.c |
Safely generates the eight-channel identity map. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| nibble_map |= 0xFFFFFFFF << (channel_count * 4); | ||
| /* Absent channel is represented as 0xf nibble. With all 8 channels present the shift count | ||
| * would be 32, which is undefined behavior for 32-bit types. | ||
| * On Xtensa and x86 architectures this would result in returning 0xffffffff, marking all |
There was a problem hiding this comment.
Is it about the line length or is the comment too extensive in general?
There was a problem hiding this comment.
For me this is okay. Not everyone may know why this behavior occurs or why this if statement is important.
There was a problem hiding this comment.
@tmleman in general, we are adding way too long comments everywhere. For example here the first 2 lines are all what we need.
There was a problem hiding this comment.
@abonislawski I would even agree with you if it weren't for the fact that a push will trigger a re-run in CI.
There was a problem hiding this comment.
IMO It is maybe too verbose but still acceptable. But since we are on this topic, I would avoid describing entire flows and referencing other function names if that's possible (especially inside lower-level functions while checking preconditions). Such descriptions could easily become obsolete after refactoring or re-design. So generally I agree with @abonislawski
bitmask_to_nibble_channel_map() fills the nibbles of absent channels with 0xf using 0xFFFFFFFF << (channel_count * 4). channel_mask comes from the host-supplied ALH multi-gateway blob and popcount() == 8 is accepted by copier_set_alh_multi_gtw_channel_map(), so a mask of 0xff makes the shift count 32, which is undefined behaviour for a 32-bit type (UBSan: "shift exponent 32 is too large for 32-bit type"). Xtensa and x86 mask the shift count to 5 bits, so the shift by 32 behaves as a shift by 0 and the map becomes 0xFFFFFFFF: every channel of an 8-channel ALH aggregation is then marked absent in copier_dai_params() instead of getting the identity map 0x76543210. Only fill absent nibbles when there are fewer than 8 channels and use an unsigned literal. Results for all masks with fewer than 8 channels are unchanged. Found by the IPC4 libFuzzer campaign with -fsanitize=undefined. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
PR 11256: test resultsRun date: 2026-10-05 17:55 UTC Tested commit: 535b305783e9ab808b043b085b7ca95ffd5ea1ee
|
bitmask_to_nibble_channel_map() fills the nibbles of absent channels with 0xf using 0xFFFFFFFF << (channel_count * 4). channel_mask comes from the host-supplied ALH multi-gateway blob and popcount() == 8 is accepted by copier_set_alh_multi_gtw_channel_map(), so a mask of 0xff makes the shift count 32, which is undefined behaviour for a 32-bit type (UBSan: "shift exponent 32 is too large for 32-bit type").
Xtensa and x86 mask the shift count to 5 bits, so the shift by 32 behaves as a shift by 0 and the map becomes 0xFFFFFFFF: every channel of an 8-channel ALH aggregation is then marked absent in copier_dai_params() instead of getting the identity map 0x76543210.
Only fill absent nibbles when there are fewer than 8 channels and use an unsigned literal. Results for all masks with fewer than 8 channels are unchanged.
Found by the IPC4 libFuzzer campaign with -fsanitize=undefined.