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.

Sign up for GitHub

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

Jump to bottom

Revise dl_iterate_phdr callback to be sound-ish #660

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Revise dl_iterate_phdr callback to be sound-ish #660

merged 2 commits into from
Aug 29, 2024

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Aug 29, 2024 •
edited
Loading

We

  • no longer pretend the entire Unixverse is GNU/Linux
  • no longer pretend static linking does not exist
  • no longer dereference pointers without checking them, except for the info object itself, where it's libc's responsibility to be sound

This passed CI on locally running rustc's arm-android Docker image.

Fixes #659.

@workingjubilee
Revise dl_iterate_phdr callback to be sound-ish
3858946 
We
- no longer pretend the entire Unixverse is GNU/Linux
- no longer pretend static linking does not exist
- no longer dereference pointers without checking them, except for
  the info object itself, where it's libc's responsibility to be sound

This passed CI on locally running rustc's arm-android Docker image.
@workingjubilee workingjubilee mentioned this pull request Aug 29, 2024
Merged
@ChrisDenton
Copy link
Member

ChrisDenton commented Aug 29, 2024 •
edited
Loading

I don't necessarily want to block this PR in particular since this is a comment that could apply to alot of code in this repo but... it feels like we should be making more use of NonNull or other abstraction for the if !ptr.is_null() { do stuff with ptr } pattern. Because like there's nothing enforcing that the null check is done on the same pointer that's being used, Any mistakes should of course be caught by the reviewer but making it machine checked would be nicer.

@nyurik nyurik mentioned this pull request Aug 29, 2024
Merged
3 tasks
@workingjubilee
Copy link
Member Author

I don't necessarily want to block this PR in particular since this is a comment that could apply to alot of code in this repo but... it feels like we should be making more use of NonNull or other abstraction for the if !ptr.is_null() { do stuff with ptr } pattern.

The particular goal here was to keep the differences with the original down to minimize the likelihood that merging this into rust-lang/rust will cause a regression that will have a very short time to root-cause and fix. I originally didn't even want to fix #659.

ChrisDenton
ChrisDenton approved these changes Aug 29, 2024
View reviewed changes
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

r=me when PR CI is done

@ChrisDenton ChrisDenton merged commit 230570f into rust-lang:master Aug 29, 2024
41 checks passed
@workingjubilee workingjubilee deleted the dont-unsoundly-iterate-phdr branch August 29, 2024 19:12
ConradIrwin pushed a commit to zed-industries/zed that referenced this pull request Sep 19, 2024
@renovate
Update Rust crate backtrace to v0.3.74 (#18039)
6670c9e 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [backtrace](https://redirect.github.com/rust-lang/backtrace-rs) |
dependencies | patch | `0.3.73` -> `0.3.74` |
| [backtrace](https://redirect.github.com/rust-lang/backtrace-rs) |
dev-dependencies | patch | `0.3.73` -> `0.3.74` |

---

### Release Notes

<details>
<summary>rust-lang/backtrace-rs (backtrace)</summary>

###
[`v0.3.74`](https://redirect.github.com/rust-lang/backtrace-rs/releases/tag/0.3.74)

[Compare
Source](https://redirect.github.com/rust-lang/backtrace-rs/compare/0.3.73...0.3.74)

#### What's Changed

- QNX Neutrino 7.0 support, thanks to
[@&#8203;nyurik](https://redirect.github.com/nyurik) in
[rust-lang/backtrace-rs#648
- Cleaned up our Android support. This should massively improve
backtraces for ones with the API level sufficient to ship with
libunwind, etc. Unfortunately, it comes at the cost of dropping support
for older ones! Thanks to
[@&#8203;fengys](https://redirect.github.com/fengys) in
[rust-lang/backtrace-rs#656
- Made PrintFmt, which was using the `Enum::__NonExhaustiveVariant`
pattern, use `#[non_exhaustive]` for real. Don't @&#8203; me if you were
matching on that! Thanks to
[@&#8203;nyurik](https://redirect.github.com/nyurik) in
[rust-lang/backtrace-rs#651
- Massively cleaned up the windows code! We moved from winapi to
windows-sys with windows-targets thanks to
[@&#8203;CraftSpider](https://redirect.github.com/CraftSpider) and
[@&#8203;ChrisDenton](https://redirect.github.com/ChrisDenton) in
- Don't cast HANDLE to usize and back by
[@&#8203;CraftSpider](https://redirect.github.com/CraftSpider) in
[rust-lang/backtrace-rs#635
- Switch from `winapi` to `windows-sys` by
[@&#8203;CraftSpider](https://redirect.github.com/CraftSpider) in
[rust-lang/backtrace-rs#641
- Update windows bindings and use windows-targets by
[@&#8203;ChrisDenton](https://redirect.github.com/ChrisDenton) in
[rust-lang/backtrace-rs#653
- A bunch of updated dependencies. Thanks
[@&#8203;djc](https://redirect.github.com/djc) and
[@&#8203;khuey](https://redirect.github.com/khuey)!
- Sorry if you were testing this code in miri! It started yelling about
sussy casts. A lot. We did a bunch of internal cleanups that should make
it quiet down, thanks to
[@&#8203;workingjubilee](https://redirect.github.com/workingjubilee) in
[rust-lang/backtrace-rs#641
- Uhhh we had to tweak `dl_iterate_phdr` in
[rust-lang/backtrace-rs#660
after Android revealed it was... kind of unsound actually and not doing
things like checking for null pointers before making slices! WHOOPS!
Thanks to [@&#8203;saethlin](https://redirect.github.com/saethlin) for
implementing detection for precisely that in rustc! It's really hard to
find soundness issues in inherited codebases like this one...

#### New Contributors

- [@&#8203;CraftSpider](https://redirect.github.com/CraftSpider) made
their first contribution in
[rust-lang/backtrace-rs#635
- [@&#8203;fengys1996](https://redirect.github.com/fengys1996) made
their first contribution in
[rust-lang/backtrace-rs#656
- [@&#8203;djc](https://redirect.github.com/djc) made their first
contribution in
[rust-lang/backtrace-rs#657

**Full Changelog**:
rust-lang/backtrace-rs@0.3.73...0.3.74

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 3pm on Wednesday" in timezone
America/New_York, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

Release Notes:

- N/A

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44MC4wIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@ChrisDenton ChrisDenton ChrisDenton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dl_iterate_phdr assumes GNU/Linux
3 participants
@workingjubilee @ChrisDenton @RalfJung