Skip to content

fix(allocator/vec2): guard against cap rather than allocation size exceeding u32::MAX #11064

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented May 15, 2025

Introduce a cap_guard to guard against cap exceeding u32::MAX and revert alloc_guard to before.

@Copilot Copilot AI review requested due to automatic review settings May 15, 2025 23:22
@Dunqing Dunqing requested a review from overlookmotel as a code owner May 15, 2025 23:22
@github-actions github-actions bot added the C-bug Category - Bug label May 15, 2025
Copy link
Member Author

Dunqing commented May 15, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request fixes the capacity guard logic in the vector allocator to check the capacity rather than the allocation size against u32::MAX. It replaces the old alloc_guard with the new cap_guard function, updates related calls in growth functions, and revises the associated comments.

  • Replace alloc_guard with cap_guard in critical allocation and growth functions.
  • Update inline documentation to accurately describe the guard behavior.
  • Adjust error handling to use unwrap_err_unchecked in from_raw_parts_in.

@Dunqing Dunqing force-pushed the 05-16-fix_allocator_vec2_guard_against_cap_rather_than_allocation_size_exceeding_u32_max branch from 63465bb to c1e4d69 Compare May 15, 2025 23:32
Copy link

codspeed-hq bot commented May 15, 2025

CodSpeed Instrumentation Performance Report

Merging #11064 will not alter performance

Comparing 05-16-fix_allocator_vec2_guard_against_cap_rather_than_allocation_size_exceeding_u32_max (97ba73b) with main (5dcd0f1)

Summary

✅ 36 untouched benchmarks

@Dunqing Dunqing force-pushed the 05-16-fix_allocator_vec2_guard_against_cap_rather_than_allocation_size_exceeding_u32_max branch 2 times, most recently from 5065d07 to 11f8c00 Compare May 15, 2025 23:43
@Dunqing Dunqing changed the title fix(allocator/vec2): guard against cap rather than allocation size exceeding u32::MAX fix(allocator/vec2): guard against cap rather than allocation size exceeding u32::MAX May 15, 2025
@Dunqing
Copy link
Member Author

Dunqing commented May 16, 2025

A little performance regression
image

@Dunqing Dunqing force-pushed the 05-16-fix_allocator_vec2_guard_against_cap_rather_than_allocation_size_exceeding_u32_max branch from 11f8c00 to 97ba73b Compare May 16, 2025 00:06
@Dunqing
Copy link
Member Author

Dunqing commented May 16, 2025

Kinda relate to this PR because too many checks in grow_* methods, as you just mentioned in the meeting, so I want to bring it up here.

Rust uses Layout::repeat rather than Layout::array<T> in grow_exact and grow_amortized, which has fewer checks. Before I want to align this part as well, but unfortunately, the Layout::repeat is unstable, but it is quite easy to copy to our own.

https://github.com/rust-lang/rust/blob/c79bbfab78dcb0a72aa3b2bc35c00334b58bfe2e/library/alloc/src/raw_vec/mod.rs#L819C1-L822C2

/cc @overlookmotel Maybe we can take this way to reduce some checks, what do you think?

@overlookmotel
Copy link
Contributor

overlookmotel commented May 16, 2025

We have 3 checks:

  • (len as usize).checked_add(additional)
  • cap_guard / alloc_guard
  • Layout::array

We should ideally try to remove at least 1 of these checks, and possibly remove 2.


We should be able to combine the last 2 checks, and create Layout without checks (Layout::from_size_align_unchecked).

impl<'a, T> RawVec<'a, T> {
    const MAX_CAP: usize = {
        let max_cap_elements = u32::MAX as usize;
        let max_cap_due_to_size = (isize::MAX as usize) / size_of::<T>();
        min(max_cap_elements, max_cap_due_to_size)
    };
}

const fn min(a: usize, b: usize) -> usize {
    if a < b { a } else { b }
}

Check if requested capacity is greater than Self::MAX_CAP, and return an error if it is.

We've now satisfied the conditions of Layout::from_size_align_unchecked, so can skip the 3rd check.

(I think this covers both 32-bit and 64-bit systems)


As to how to remove the checked_add too, we already have an example in RawVec we could adapt. Note that this does not use checked arithmetic because overflow is impossible:

fn needs_to_grow(&self, len: u32, additional: usize) -> bool {
// `self.cap().wrapping_sub(len) as usize` is safe because
// `self.cap()` is `u32` and `len` is `u32`, so the result
// is guaranteed to be less than `usize::MAX`.
additional > self.cap().wrapping_sub(len) as usize
}

This last one may or may not be a perf gain. We remove one branch but the arithmetic takes more operations.

@Dunqing
Copy link
Member Author

Dunqing commented May 17, 2025

@overlookmotel Thank you for reviewing this. I intend to work on it when you finish your work on Vec so that we don't conflict, or if you want, feel free to take it over.

@overlookmotel
Copy link
Contributor

I'm pretty much done with follow-up PRs about Vec now. The stack up to #11081 is ready to review and merge, if you're OK with the changes. The last 2 PRs in that stack I'm less sure about - they were experiments really, but they shouldn't block other work on Vec.

Concerning this PR: I'd be happy to leave it to you, if that's OK.

@overlookmotel
Copy link
Contributor

Ah I see you've already reviewed most of that stack. I'll merge them now. Please review the last one #11081 when you get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category - Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants