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

fn AlignedVec::resize: Validate safety requirements, specifically overflow #1357

Merged

Conversation

HeroicKatora
Copy link
Contributor

@HeroicKatora HeroicKatora commented Sep 10, 2024

This is a necessary check for soundness, as demonstrated by the test which can SIGSEGV without the check. Before the check, an overflow in the underlying buffer calculation can create incoherent state where the vector believes in an impossibly large buffer of the item type which is not actually backed by a correctly sized buffer of chunks.

src/align.rs Outdated Show resolved Hide resolved
@kkysen kkysen self-requested a review September 10, 2024 16:37
@kkysen kkysen self-assigned this Sep 10, 2024
This is a necessary check for soundness, as demonstrated by the test
which can SIGSEGV without the check. Before the check, an overflow in
the underlying buffer calculation can create incoherent state where the
vector believes in an impossibly large buffer of the item type which is
not actually backed by a correctly sized buffer of chunks.
@kkysen kkysen changed the title Validate AlignedVec::resize safety requirements fn AlignedVec::resize: Validate safety requirements, specifically overflow Sep 17, 2024
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Thanks for finding this bug and the PR!

src/align.rs Outdated Show resolved Hide resolved
src/align.rs Outdated Show resolved Hide resolved
src/align.rs Outdated Show resolved Hide resolved
src/align.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

LGTM now except for that one little comment. Appreciate the NOTEs, too.

src/align.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

LGTM now. @HeroicKatora, I'll merge it now assuming you're all done with it. Thanks again!

@kkysen kkysen merged commit b417059 into memorysafety:main Sep 23, 2024
28 checks passed
@HeroicKatora HeroicKatora deleted the issue-1356-aligned-vec-overflow branch September 23, 2024 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overflows can unsoundly taint length state in AlignedVec
2 participants