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

Improve handling of failed operations in isolation mode #1034

Open
Aaron1011 opened this issue Nov 5, 2019 · 21 comments
Open

Improve handling of failed operations in isolation mode #1034

Aaron1011 opened this issue Nov 5, 2019 · 21 comments
Labels
A-shims Area: This affects the external function shims A-ux Area: This affects the general user experience C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Nov 5, 2019

Currently, we abort program execution when we try to access the external environment in isolation mode. However, libstd currently calls getcwd when printing out a panic backtrace. To ensure that panicking works in isolation mode, we've disabled the call to getcwd when libstd has cfg(miri) enabled.

Eventually, it would be good to remove this hack, and start returning proper error codes from disabled functions, rather than aborting the process.

See rust-lang/rust#60026 (comment) for more discussion

@RalfJung RalfJung added A-shims Area: This affects the external function shims A-ux Area: This affects the general user experience C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Nov 5, 2019
@RalfJung
Copy link
Member

RalfJung commented Nov 5, 2019

@oli-obk I'd be interested in your opinion here: right now we abort execution on shims that are unsupported due to isolation; if we instead continue execution pretending the function errored that might be unexpected. Though OTOH this is what "normal" sandboxes do so I think it's actually expected. We could emit a warning the first time this happens -- this might be a good test project for #797.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 5, 2019

hmm... we do this already for all the pthread code, so we do have precedent. I'm fine with it and I don't think we'd need a warning. I would expect a sandboxed open call to error after all.

@Aaron1011
Copy link
Member Author

Aaron1011 commented Nov 5, 2019

If we decide to make disabled functions return an error code, I think we should have a command-line flag to opt in to the old behavior. There should be a way of running Miri such that either:

  1. The program output is identical to the output when run normally (i.e. compiled by rustc and executed), or:
  2. We terminate the program with an explicit Miri error.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 6, 2019

While I personally don't see a reason for such a flag, I won't block it if there is strong desire for it.

We've been doing quite well with just having unsupported functions return even success return values and do nothing. Going a step further and reporting error return values for unsupported functions where the caller has a fallback or otherwise useful code path seems totally alright to me.

@RalfJung
Copy link
Member

RalfJung commented Nov 6, 2019

While I personally don't see a reason for such a flag, I won't block it if there is strong desire for it.

Same.

The program output is identical to the output when run normally (i.e. compiled by rustc and executed)

Note that we cannot guarantee this due to things like pointer-integer casts, or libc functions that return the stack size (we just return some constant, 16k if I remember correctly). And we already don't try to be faithful for most things "implemented" in foreign_items.rs, we just make these good enough such that things work.

@RalfJung
Copy link
Member

Here's another example where it would be good if isolated operations returned failure instead of aborting execution: seanmonstar/num_cpus#96 makes num_cpus open a file (rejected with isolation) but falls back to another way of determining the CPU count (which works in isolation).

@RalfJung RalfJung added the E-good-first-issue A good way to start contributing, mentoring is available label Sep 28, 2020
@camshaft
Copy link

This would be great to have for my usecase. I'm wanting to add miri support to https://github.com/camshaft/bolero and wanted to read the fuzz corpus from the filesystem, if possible, otherwise fall back to only generating some random tests cases with a warning saying to disable isolation if desired.

@camelid
Copy link
Member

camelid commented Oct 24, 2020

I'm going to try working on this.

@RalfJung
Copy link
Member

@camelid that's great. :-)

@henryboisdequin
Copy link
Contributor

@camelid Are you still working on this or can I take a shot at this?

@camelid
Copy link
Member

camelid commented Mar 3, 2021

@henryboisdequin I never ended up getting to this, so you can try!

@atsmtat
Copy link
Contributor

atsmtat commented May 14, 2021

Hi @RalfJung , I opened a PR fixing get and set cwd ops. Let me know what you think.
Also, this is my first PR in rustc related packages. If you find something missing from the usual workflow, please let me know 🙂

@RalfJung
Copy link
Member

RalfJung commented Jun 9, 2021

With #1797 we now support returning EPERM for some operations when isolation is enabled, instead of aborting execution. What is left to do is do this for as many operations as possible. :) It might be good to assemble a list.

@atsmtat
Copy link
Contributor

atsmtat commented Jun 11, 2021

I guess most of the fs operations can return EPERM.

@RalfJung
Copy link
Member

Yes, I think that would make a lot of sense.

@atsmtat
Copy link
Contributor

atsmtat commented Jun 16, 2021

Yes, I think that would make a lot of sense.

#1838 takes care of fs shims.

bors added a commit that referenced this issue Jul 25, 2021
Fix use of deprecated `check_no_isolation` in posix fs shims

Update posix fs shims to use new API `reject_in_isolation`, which
allows rejection with error code instead of always forcing abort.
Error code chosen for each op is the most appropriate one from the
list in corresponding syscall's manual.

Updated helper APIs to not use quotes (\`) around input name while
preparing the message. This allows callers to pass multi-word string
like -- "\`read\` from stdin".

Cc #1034
@RalfJung
Copy link
Member

With #1838 landing soon, the remaining uses of check_no_isolation seem to be

  • the shims in time.rs
  • pthread_cond_timedwait
  • FUTEX syscall with a timeout

@yunji-yunji

This comment was marked as off-topic.

@saethlin

This comment was marked as off-topic.

@yunji-yunji

This comment was marked as off-topic.

@RalfJung

This comment was marked as off-topic.

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 A-ux Area: This affects the general user experience C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available
Projects
None yet
Development

No branches or pull requests

9 participants