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

regression: misaligned pointer dereference: address must be a multiple ... #111487

Closed
Mark-Simulacrum opened this issue May 12, 2023 · 7 comments
Closed
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

A bunch of these seem to come from rand_core 0.2.1, 0.3.0, and 0.4.0 code (not sure if we can fix it there?)

Not rand_core:

cc #98112

@Mark-Simulacrum Mark-Simulacrum added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label May 12, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 12, 2023
@Mark-Simulacrum Mark-Simulacrum added this to the 1.71.0 milestone May 12, 2023
@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 12, 2023
@Mark-Simulacrum
Copy link
Member Author

#98112 doesn't seem to have an FCP for doing this, which makes some sense (it's just "exploiting" UB).

@saethlin did we file PRs against the relevant crates which break the most transitive tests at least? e.g., rand_core 0.2.x, 0.3.x, 0.4.x seem to be a good fraction of this list. There's a few more crates that seem to stand out too (e.g., plotters-bitmap).

@saethlin
Copy link
Member

We did not make any effort to file fixes for the relevant crates.

The rand_core issue in 0.4 has been patched for about 4 years: rust-random/rand#783 then it looks to me like rand does the semver trick, so I think they pulled the fix into 0.3 and 0.2, based on spot checking.

The plotters-bitmap issue was patched a few weeks ago, it's just not released yet: plotters-rs/plotters#467

@saethlin
Copy link
Member

The only things I think are significant are:

An issue in an old version (almost 4 years old) of wasmtime-runtime, https://crater-reports.s3.amazonaws.com/beta-1.70-2/beta-2023-05-08/gh/rohankumardubey.wizer/log.txt

Multiple crates hitting an issue in kamadak-exif-0.3.1:
https://crater-reports.s3.amazonaws.com/beta-1.70-2/beta-2023-05-08/reg/quad-image-0.1.1/log.txt
https://crater-reports.s3.amazonaws.com/beta-1.70-2/beta-2023-05-08/gh/jondot.rawsort/log.txt
https://crater-reports.s3.amazonaws.com/beta-1.70-2/beta-2023-05-08/gh/senden9.geo_fence/log.txt
This issue was patched 4 years ago in kamadak/exif-rs@288a7e8 but there isn't a semver-compatible update available.

I don't think these are worth doing anything with. All we could do here is ask maintainers to backport fixes.

@Mark-Simulacrum
Copy link
Member Author

Sounds like that's right. The main thing we usually try to ensure is that there is some upgrade path, especially for common code, and I think this covers most of that. Plus it technically won't affect non-debug builds, so users have that escape hatch too.

I'm fine leaving this as-is.

@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.71.0, 1.70.0 May 13, 2023
@Mark-Simulacrum Mark-Simulacrum removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 13, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue May 28, 2023
Don't check for misaligned raw pointer derefs inside Rvalue::AddressOf

From rust-lang#112026 (comment):

rustc 1.70 (stable next week) added a Mir pass to add pointer alignment checks in debug mode. Adding these checks caused some crates to break, but that was expected, since they contain broken code (rust-lang#111487) for tracking that.

However, the checks added are slightly more aggressive than they should have been. Specifically, they also check the place in an `addr_of!` expression. Whether lack of alignment there is or isn't UB is unclear. This PR modifies the pass to not affect those cases.

I spot checked the crater regressions and the ones I saw were not the case that this PR is modifying. It still seems good to not land anything overaggressive though
saethlin pushed a commit to saethlin/miri that referenced this issue May 29, 2023
Don't check for misaligned raw pointer derefs inside Rvalue::AddressOf

From rust-lang/rust#112026 (comment):

rustc 1.70 (stable next week) added a Mir pass to add pointer alignment checks in debug mode. Adding these checks caused some crates to break, but that was expected, since they contain broken code (rust-lang/rust#111487) for tracking that.

However, the checks added are slightly more aggressive than they should have been. Specifically, they also check the place in an `addr_of!` expression. Whether lack of alignment there is or isn't UB is unclear. This PR modifies the pass to not affect those cases.

I spot checked the crater regressions and the ones I saw were not the case that this PR is modifying. It still seems good to not land anything overaggressive though
@oherrala
Copy link

oherrala commented Jun 2, 2023

I think ipconfig crate is affected by this: liranringel/ipconfig#53

thread '<redacted>' panicked at 'misaligned pointer dereference: address must be a multiple of 0x8 but is 0x189f604', C:\Users\runneradmin\.cargo\registry\src\index.crates.io-1cd66030c949c28d\ipconfig-0.3.1\src\adapter.rs:293:23

@saethlin
Copy link
Member

saethlin commented Jun 2, 2023

@Mark-Simulacrum Should this issue stay open? I don't think we have an actual regression here.

@Mark-Simulacrum
Copy link
Member Author

Yeah, I think we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants