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

Some direct memory accesses lack bounds-checks #1007

Closed
3 tasks done
RalfJung opened this issue Oct 19, 2019 · 6 comments · Fixed by #1022
Closed
3 tasks done

Some direct memory accesses lack bounds-checks #1007

RalfJung opened this issue Oct 19, 2019 · 6 comments · Fixed by #1022
Labels
A-shims Area: This affects the external function shims C-bug Category: This is a bug.

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 19, 2019

The Allocation APIs should generally be avoided as they are low-level building blocks and don't do bounds or alignment checks themselves. I did an audit and noticed some uses crept in recently that are not properly checked:

  • getcwd
  • read
  • write

This can lead to ICEs. When these are fixed, a test should be added.

Cc @christianpoveda @oli-obk

It would be even better if we could stop exposing Memory::get and Memory::get_mut from librustc_mir... once these issues are fixed and the current wave of PRs is in (I suggested to @christianpoveda to add a write_bytes API anyway), we could try to make a concerted effort to remove the remaining uses in Miri.

@RalfJung RalfJung added A-shims Area: This affects the external function shims C-bug Category: This is a bug. labels Oct 19, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Oct 19, 2019

So is the goal to make these methods private? Or rename to get_internal or sth?

@RalfJung
Copy link
Member Author

Making them private would be great.
Renaming them could be a good idea nevertheless.

@pvdrz
Copy link
Contributor

pvdrz commented Oct 20, 2019

I can do a PR with the changes after rust-lang/rust#65621 lands

@RalfJung
Copy link
Member Author

RalfJung commented Oct 21, 2019

The getcwd one should actually be taken care of in #993 -- as in, I will not accept that PR until it fixes this issue. ;)

@pvdrz
Copy link
Contributor

pvdrz commented Oct 21, 2019

Hahaha yes i know about that one :P. I was talking about read and write.

@RalfJung
Copy link
Member Author

Status update: getcwd is fixed. @christianpoveda is on read and write.

While at it, I think you can now also avoid the remove_handle_and-and-then-insert dance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shims Area: This affects the external function shims C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants