Safely read from slices in wrap_frame

[Brendonovich]

Summary by CodeRabbit

• Bug Fixes
  • Resolved potential stability issues in audio frame processing with improved channel configuration handling, enhanced buffer boundary validation, and safer memory management to prevent crashes and data corruption across different audio channel configurations.

[coderabbitai]

Walkthrough

The wrap_frame_with_max_channels function in the media-info library was reworked to respect allocated output channels rather than provided max_channels, replacing direct slice copies with bounds-checked operations across mono/packed and multi-channel de-interleaving code paths.

Changes

Cohort / File(s)	Summary
Buffer boundary safety fixes crates/media-info/src/lib.rs	Reworked wrap_frame_with_max_channels to use allocated out_channels instead of max_channels; added bounds-checked copying with calculated limits and range validation; replaced unsafe direct slice copies with conditional, safe operations in both mono/packed and de-interleaving paths; added clarifying comment on frame allocation safety

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Verify bounds-checking logic in mono/packed path handles edge cases correctly
• Confirm calculated copy_len and range validation prevent out-of-bounds access in packed channel branch
• Validate de-interleaving loop changes with get_mut and range checks work for all channel configurations
• Ensure no legitimate use cases are broken by the out_channels vs max_channels distinction

Poem

🐰 Hopping through the channels with care,
Bounds checked here and everywhere,
No buffer overflow shall we fear,
Safe copy paths are crystal clear!
Allocations respected, bugs all fixed,
In this data path remix! 🎵

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "Safely read from slices in wrap_frame" is directly related to and accurately summarizes the main changes in the pull request. The changeset focuses on reworking the wrap_frame_with_max_channels function to add explicit bounds checking and safe copying operations—replacing direct slice access with conditional, bounds-checked copies and range checks throughout multiple code paths. The title is concise, specific to the function being modified, avoids vague terminology or noise, and clearly communicates the primary change (improving safety of slice operations) that a teammate scanning history would immediately understand.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch safer-wrap-frame

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

crates/media-info/src/lib.rs (3)

160-172: LGTM: Correct bounds-checked channel reduction for packed data.

The logic correctly reduces channels by:

1. Calculating the output position based on out_channels (line 162)
2. Copying only the first out_channels worth of samples from each input chunk (line 164)
3. Using bounds-checked operations to prevent panics (lines 166-171)

The mathematics is sound: each output chunk of sample_size * out_channels bytes fits within the allocated frame buffer.

Optional: Consider logging when bounds checks fail.

If the get()/get_mut() calls return None, the copy is silently skipped. While this prevents panics, it could hide calculation errors during development. Consider adding a debug assertion or warning log:

                 if let (Some(chunk_slice), Some(frame_slice)) = (
                     packed_chunk.get(0..copy_len),
                     frame.data_mut(0).get_mut(start..start + copy_len),
                 ) {
                     frame_slice.copy_from_slice(chunk_slice);
+                } else {
+                    debug_assert!(false, "Bounds check failed in packed channel reduction");
                 }

---

173-188: LGTM: Correct bounds-checked de-interleaving for planar data.

The planar data path correctly:

1. Limits the channel loop to out_channels (line 177), preventing writes to unallocated channels
2. Extracts the appropriate samples for each channel from the packed input (lines 178-179)
3. Uses bounds-checked operations to safely copy data (lines 180-185)

The indexing is sound: each channel receives samples worth of data at the correct planar positions.

Optional: Consider logging when bounds checks fail.

Similar to the packed case, silent failures on bounds checks could hide bugs:

                     if let (Some(chunk_slice), Some(frame_slice)) = (
                         packed_chunk.get(channel_start..channel_end),
                         frame.data_mut(channel).get_mut(start..start + sample_size),
                     ) {
                         frame_slice.copy_from_slice(chunk_slice);
+                    } else {
+                        debug_assert!(false, "Bounds check failed in planar de-interleaving");
                     }

---

330-383: LGTM: Good test coverage for main scenarios.

The tests effectively cover:

• Both packed and planar formats
• Normal wrapping without channel reduction
• Channel reduction via max_channels parameter
• Correct data extraction in all cases

Optional: Consider adding edge case tests.

While current coverage is solid, consider tests for:

• Empty or single-sample inputs
• Boundary conditions (e.g., MAX_AUDIO_CHANNELS)
• Mono audio (1 channel) which takes a special fast path

Example:

#[test]
fn wrap_mono_frame() {
    let info = AudioInfo::new_raw(Sample::U8(Type::Packed), 2, 1);
    let input = &[1, 2];
    let frame = info.wrap_frame(input);
    assert_eq!(&frame.data(0)[0..input.len()], input);
}

#[test]
fn wrap_empty_frame() {
    let info = AudioInfo::new_raw(Sample::U8(Type::Packed), 2, 2);
    let input = &[];
    let frame = info.wrap_frame(input);
    // Verify no panic occurs
}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf5fda5 and 64af6e5.

📒 Files selected for processing (1)

• crates/media-info/src/lib.rs (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust code using rustfmt and ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.

Files:

• crates/media-info/src/lib.rs

crates/*/src/**/*

📄 CodeRabbit inference engine (AGENTS.md)

Rust crates should place tests within the src/ and/or a sibling tests/ directory for each crate inside crates/*.

Files:

• crates/media-info/src/lib.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Clippy
• GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
• GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
• GitHub Check: Analyze (rust)

🔇 Additional comments (2)

crates/media-info/src/lib.rs (2)

142-142: LGTM: Correct channel capping.

The out_channels calculation correctly ensures the output respects the max_channels limit while never exceeding the available input channels. This value is consistently used for frame allocation and subsequent operations.

---

157-159: LGTM: Safe direct copy for non-reducing cases.

The condition correctly identifies cases where no channel reduction occurs:

• Mono audio (1 channel) can be copied directly
• Packed frames where self.channels <= out_channels means self.channels == out_channels (since out_channels = min(self.channels, max_channels)), so all data fits

The frame allocation and data sizes align correctly in both cases.