[REBASE & FF] Enable Clippy's Deny indexing_slicing Lint by os-d · Pull Request #1517 · OpenDevicePartnership/patina

os-d · 2026-05-21T15:02:33Z

Description

This enables an optional clippy lint to disallow direct slice indexing. First, all instances in the codebase are fixed or explicitly allowed.

The only places they are explicitly allowed are:

In const functions because .get() is currently not const. An unstable feature could be enabled to allow this, but because const functions are evaluated at build time, it did not seem worthwhile to enable
In test code

Each crate was updated separately to make any future bisects easier to identify where an issue occurred and to allow reviewers to focus on crates they care about.

How This Was Tested

Unit tests, booting to Win/Linux on Q35/SBSA and Windows on a physical Intel platform.

Integration Instructions

Clippy will now fail when direct slice indexing is used. Developers must either use .get() (preferred) or disable the lint at the smallest level possible if it is not possible to use .get().

patina-automation · 2026-05-21T15:02:47Z

✅ QEMU Validation Passed

All QEMU validation jobs completed successfully.

Note: Q35 is only built on Windows hosts (QEMU boot is disabled due to a QEMU vfat issue).

Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/26253306380

Boot Time to EFI Shell

Platform	Elapsed
Q35 (Linux Host)	27.0s
SBSA (Linux Host)	33.6s

Dependencies

Repository	Ref
patina	`373e9e3`
patina-dxe-core-qemu	`af9c5f5`
patina-fw-patcher	`d6d246b`
patina-qemu firmware	`v3.0.0`
patina-qemu build script	`965bc5f`

This comment was automatically generated by the Patina QEMU PR Validation Post workflow.

os-d · 2026-05-21T15:39:26Z

Hmm, the clippy logic with all targets is not working correctly because it didn't check all the arm64 code. Will run on an arm64 machine and fix those up; will also take a look at why clippy with all targets isn't working here

In preparation for enabling the clippy lint to disable direct slice indexing, fix up instances in patina_acpi. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation to enable the clippy lint disallowing direct slice indexing, don't do it in patina_adv_logger. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint disallowing direct slice indexing, don't use it in patina_mm. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint disallowing direct slice indexing, don't use it in patina_performance. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_debugger. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_internal_collections. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_internal_cpu. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_internal_depex. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_stacktrace. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation to enable the clippy lint that disallows direct slice indexing, don't do it in patina_dxe_core. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't use it in the SDK. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_ffs. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_macro. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_ffs_extractors. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

All code has been updated to not use direct slice indexing or allow it for specific cases. Enable the lint. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

codecov · 2026-05-21T21:02:08Z

Codecov Report

❌ Patch coverage is 91.36069% with 40 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...a_dxe_core/src/pi_dispatcher/section_decompress.rs	0.00%	10 Missing ⚠️
components/patina_adv_logger/src/logger.rs	0.00%	6 Missing ⚠️
components/patina_mm/src/config.rs	79.17%	5 Missing ⚠️
patina_dxe_core/src/pi_dispatcher/image.rs	60.00%	4 Missing ⚠️
sdk/patina/src/pi/fw_fs.rs	92.50%	3 Missing ⚠️
sdk/patina_ffs/src/file.rs	86.36%	3 Missing ⚠️
sdk/patina_ffs/src/volume.rs	90.91%	3 Missing ⚠️
patina_dxe_core/src/allocator.rs	33.33%	2 Missing ⚠️
...ts/patina_performance/src/component/performance.rs	80.00%	1 Missing ⚠️
core/patina_internal_depex/src/lib.rs	80.00%	1 Missing ⚠️
... and 2 more

📢 Thoughts on this report? Let us know!

Javagedes · 2026-05-21T21:34:43Z


        log::debug!(target: "mm_comm", "Outgoing MM communication request: buffer_id={}, data_size={}, recipient={:?}", id, data_buffer.len(), recipient);
-        log::debug!(target: "mm_comm", "Request Data (hex): {:02X?}", &data_buffer[..core::cmp::min(data_buffer.len(), 64)]);
+        log::debug!(target: "mm_comm", "Request Data (hex): {:02X?}", data_buffer.get(..core::cmp::min(data_buffer.len(), 64)).unwrap_or(data_buffer));


This one seems like slicing is acceptable. I have only started scrolling through this code so I'm not sure if you are making some exceptions with allow(clippy::indexing_slicing)

My general pattern was to not allow direct slice indexing except for cases where we had to, i.e. const functions. Otherwise, only tests were allowed to have it.

There are a lot of places where it is acceptable because of checks being done, but I tried to have migrate to using .get() and dropping the redundant checks in the code. This reduces the number of allows required and then also the .get() stays with the slice if you move logic around, but you may accidentally move the slice indexing before the check and blow up, etc.

So, I'm not opposed to allow direct slice indexing some places, but I do think we should be judicious in where we choose to. I would prefer to have a reason to allow it, i.e. perf, instead of having a reason to not use it.

For this case, it's a debug print that isn't compiled in most of the time, I'd just leave .get and say, yeah, it's a little silly it does .get() on a min of the buffer len, but who cares. But I don't have a strong feeling on it.

Javagedes · 2026-05-21T21:38:38Z

        let rec_len = header.length as usize;
        if rec_len < PerformanceRecordHeader::SIZE {
-            self.bytes = &self.bytes[PerformanceRecordHeader::SIZE..];
+            self.bytes = self.bytes.get(PerformanceRecordHeader::SIZE..).unwrap_or(&[]);


I'm wondering if its better to leave self.bytes untouched if we cannot get the bytes. something like

if rec_len < PerformanceRecordHeader::SIZE and let Some(bytes) = self.bytes.get(PerformanceRecordHeader::SIZE..) { self.bytes = bytes; }

Javagedes · 2026-05-21T21:45:44Z

-            node.set_right(Some(next));
-            next.set_left(Some(node));
-            node = next;
+        for pair in buffer.windows(2) {


I think this is cleaner:

for [node, next] in buffer.array_windows::<2>() { node.set_right(Some(next)); next.set_left(Some(node)); }

Javagedes · 2026-05-21T21:48:12Z

            Self::build_linked_list(self.data);
-            self.available.set(self.data[0].as_mut_ptr());
+            // first() is safe: we just assigned a non-empty buffer (assert above ensures len >= capacity).
+            self.available.set(self.data.first().expect("non-empty buffer").as_mut_ptr());


This one seems unnecessary and results in a safety check followed by an unwrap uncheck. The performance difference is minimal but still not sure if this helps anything at all.

Javagedes · 2026-05-21T21:52:29Z

        }

-        let Err(idx) = self.search(elements[0]) else {
+        let first = *elements.first().ok_or(Error::NotSorted)?;


These new deref's make me nervous as to if we are copying or dereferencing.

Javagedes · 2026-05-21T22:58:28Z

@@ -123,18 +124,28 @@ where
    ///
    /// Returns the exact datum if it exists, or the closest datum that is greater than the key if it does not.
    pub fn search_with_key(&self, key: &T::Key) -> Result<&T, &T> {


to keep the immutable and mutable functions almost the same, maybe we do something like below. the self.is_empty() assert should make sure you can index without using get() you could still use get and "expect" if you wanted, to provide a better error message. Otherwise, we need to change the interface for this function IMO.

pub fn search_with_key(&self, key: &T::Key) -> Result<&T, &T> { assert!(!self.is_empty()); match self.binary_search_by_key(key, |e| e.key()) { Ok(idx) => Ok(&self[idx]), Err(idx) => { // Clamp to last valid index when binary_search returns past-the-end. let idx = idx.min(self.len().saturating_sub(1)); Err(&self[idx]) } } } pub fn search_with_key_mut(&mut self, key: &T::Key) -> Result<&mut T, &mut T> { match self.binary_search_by_key(key, |e| e.key()) { Ok(idx) => Ok(&mut self[idx]), Err(idx) => { // Clamp to last valid index when binary_search returns past-the-end. let idx = idx.min(self.len().saturating_sub(1)); Err(&mut self[idx]) } } }

makubacki · 2026-05-22T02:21:53Z

+            if let Some(dest) = self.buffer.get_mut(self.pos..self.pos + len) {
+                dest.copy_from_slice(data);
+            }
            self.pos += len;


It seems like something more should be done if the copy cannot be performed than just bump self.pos.

makubacki · 2026-05-22T02:23:52Z

+            if let Some(dest) = self.buffer.get_mut(self.buffer_size..self.buffer_size + len) {
+                dest.copy_from_slice(data);
+            }
            self.buffer_size += len;


Similar to the case in monitor.rs, it seems strange to keep incrementing self.buffer_size here if the copy was not possible.

makubacki · 2026-05-22T02:31:01Z

+        if let Some(slot) = self.values.get_mut(index) {
+            *slot = Some(value);
+        }


This code is a little more verbose/less readable. It seems like the resize_with() call should catch any practical problems. Is that your understanding?

makubacki · 2026-05-22T02:39:01Z

@@ -293,7 +293,7 @@ impl Registers for X64CoreRegs {
                    return Err(());
                }
                let mut array = [0u8; core::mem::size_of::<$t>()];


Can this follow what was done for the read!() macro in core\patina_debugger\src\arch\aarch64.rs?

Javagedes · 2026-05-22T15:47:25Z

 }

 #[cfg(test)]
+#[allow(clippy::indexing_slicing)]


I believe you can put this in a clippy.toml so that you don't need to do this on all the tests:

# Workspace clippy.toml allow-indexing-slicing-in-tests = true

os-d requested review from Javagedes, cfernald, joschock, makubacki and vineelko May 21, 2026 15:02

github-actions Bot added the impact:security Has a security impact label May 21, 2026

os-d commented May 21, 2026

View reviewed changes

Comment thread components/patina_acpi/src/acpi.rs

joschock approved these changes May 21, 2026

View reviewed changes

Comment thread patina_dxe_core/src/allocator/fixed_size_block_allocator.rs Outdated

os-d added 15 commits May 21, 2026 10:45

patina_acpi: Don't Use Direct Slice Indexing

51640c5

In preparation for enabling the clippy lint to disable direct slice indexing, fix up instances in patina_acpi. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_adv_logger: Don't Use Direct Slice Indexing

f15e54d

In preparation to enable the clippy lint disallowing direct slice indexing, don't do it in patina_adv_logger. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_mm: Don't Use Direct Slice Indexing

ea10366

In preparation for enabling the clippy lint disallowing direct slice indexing, don't use it in patina_mm. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_performance: Don't Use Direct Slice Indexing

8e79dba

In preparation for enabling the clippy lint disallowing direct slice indexing, don't use it in patina_performance. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_debugger: Don't Use Direct Slice Indexing

c27db97

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_debugger. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_internal_collections: Don't Use Direct Slice Indexing

a0bea76

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_internal_collections. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_internal_cpu: Don't Use Direct Slice Indexing

811df85

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_internal_cpu. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_internal_depex: Don't Use Direct Slice Indexing

854987f

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_internal_depex. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_stacktrace: Don't Use Direct Slice Indexing

17baf51

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_stacktrace. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_dxe_core: Don't Use Direct Slice Indexing

6237d6c

In preparation to enable the clippy lint that disallows direct slice indexing, don't do it in patina_dxe_core. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

Patina SDK: Don't Use Direct Slice Indexing

07cd8d9

In preparation for enabling the clippy lint that disallows direct slice indexing, don't use it in the SDK. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_ffs: Don't Use Direct Slice Indexing

0c6218b

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_ffs. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_macro: Don't Use Direct Slice Indexing

45226eb

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_macro. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

patina_ffs_extractors: Don't Use Direct Slice Indexing

0b256fa

In preparation for enabling the clippy lint that disallows direct slice indexing, don't do it in patina_ffs_extractors. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

Cargo.toml: Enable Clippy Lint to Disallow Direct Slice Indexing

373e9e3

All code has been updated to not use direct slice indexing or allow it for specific cases. Enable the lint. Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

os-d force-pushed the get_lint branch from 7919bfa to 373e9e3 Compare May 21, 2026 20:56

Javagedes reviewed May 21, 2026

View reviewed changes

makubacki reviewed May 22, 2026

View reviewed changes

Javagedes reviewed May 22, 2026

View reviewed changes

Conversation

os-d commented May 21, 2026

Description

How This Was Tested

Integration Instructions

Uh oh!

patina-automation Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ QEMU Validation Passed

Boot Time to EFI Shell

Dependencies

Uh oh!

Uh oh!

Uh oh!

os-d commented May 21, 2026

Uh oh!

codecov Bot commented May 21, 2026

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patina-automation Bot commented May 21, 2026 •

edited

Loading