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

Fix misaligned loads when loading UEFI arg pointers #122164

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

beetrees
Copy link
Contributor

@beetrees beetrees commented Mar 8, 2024

Currently, the two UEFI argument pointers are stored in an alloca of alignment 1, a pointer to which is then passed as argv. However, the library code treats argv as a pointer to an array of pointers and dereferences it as such, meaning that it presumes the alloca is aligned to at least the alignment of a pointer. This PR fixes this mismatch by aligning the alloca to the alignment of a pointer.

This PR also changed the gep to use the new inbounds_ptradd method.

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 8, 2024
@lcnr
Copy link
Contributor

lcnr commented Mar 8, 2024

r? compiler

@rustbot rustbot assigned estebank and unassigned lcnr Mar 8, 2024
@workingjubilee
Copy link
Member

@beetrees Thoughts on this versus simply making the ptr reads a read_unaligned()?

@beetrees
Copy link
Contributor Author

beetrees commented Mar 8, 2024

@workingjubilee I did consider using read_unaligned() instead, but argv is a pointer-aligned array of pointers on more "conventional" platforms too so doing it this way seems more consistent.

@workingjubilee
Copy link
Member

sgtm.

@Ayush1325 sorry for not catching this.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 8, 2024

📌 Commit 4bef0cc has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#121201 (align_offset, align_to: no longer allow implementations to spuriously fail to align)
 - rust-lang#122076 (Tweak the way we protect in-place function arguments in interpreters)
 - rust-lang#122100 (Better comment for implicit captures in RPITIT)
 - rust-lang#122157 (Add the new description field to Target::to_json, and add descriptions for some MSVC targets)
 - rust-lang#122164 (Fix misaligned loads when loading UEFI arg pointers)
 - rust-lang#122171 (Add some new solver tests)
 - rust-lang#122172 (Don't ICE if we collect no RPITITs unless there are no unification errors)
 - rust-lang#122197 (inspect formatter: add braces)
 - rust-lang#122198 (Remove handling for previously dropped LLVM version)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2b6ae95 into rust-lang:master Mar 8, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Rollup merge of rust-lang#122164 - beetrees:uefi-argv-align, r=workingjubilee

Fix misaligned loads when loading UEFI arg pointers

Currently, the two UEFI argument pointers are stored in an `alloca` of alignment 1, a pointer to which is then passed as `argv`. However, [the library code](https://github.com/rust-lang/rust/blob/9c3ad802d9b9633d60d3a74668eb1be819212d34/library/std/src/sys/pal/uefi/mod.rs#L60-L61) treats `argv` as a pointer to an array of pointers and dereferences it as such, meaning that it presumes the `alloca` is aligned to at least the alignment of a pointer. This PR fixes this mismatch by aligning the `alloca` to the alignment of a pointer.

This PR also changed the `gep` to use the new `inbounds_ptradd` method.
@beetrees beetrees deleted the uefi-argv-align branch March 11, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants