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 use of deprecated check_no_isolation in posix fs shims #1838

Merged
merged 3 commits into from
Jul 25, 2021

Conversation

atsmtat
Copy link
Contributor

@atsmtat atsmtat commented Jun 16, 2021

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

src/shims/posix/fs.rs Outdated Show resolved Hide resolved
src/shims/posix/fs.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Could you also add a test that exercises these functions? Something like tests/run-pass/current_dir_with_isolation.rs (but it doesn't need to do that loop, calling the function a single time suffices).

@RalfJung
Copy link
Member

@atsmtat it's been a while since we heard from you, what is the status of this PR?

I'm afraid there will probably be a lot of conflicts with #1851.

@atsmtat
Copy link
Contributor Author

atsmtat commented Jul 18, 2021

@atsmtat it's been a while since we heard from you, what is the status of this PR?

I'm afraid there will probably be a lot of conflicts with #1851.

Sorry for the late reply. I got busy. I addressed your comments in a new commit I just pushed.

I can wait for your PR to merge and then rebase and resolve conflicts.

@atsmtat atsmtat force-pushed the fs-isolation branch 2 times, most recently from 98f8815 to a9872e5 Compare July 18, 2021 22:17
@atsmtat
Copy link
Contributor Author

atsmtat commented Jul 18, 2021

@atsmtat it's been a while since we heard from you, what is the status of this PR?
I'm afraid there will probably be a lot of conflicts with #1851.

Sorry for the late reply. I got busy. I addressed your comments in a new commit I just pushed.

I can wait for your PR to merge and then rebase and resolve conflicts.

Just noticed that your PR is merged. I rebased my branch and resolved conflicts, wasn't too bad 🙂

src/shims/posix/fs.rs Outdated Show resolved Hide resolved
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".
Change the code to either `EACCES` (if the op is performed on the
path), or `EBADF` (if the op is performed the fd)

Updated ops: `stat`, `opendir`, `ftruncate64`, and `readlink`

Add a new test for fs ops in isolation.
@atsmtat atsmtat force-pushed the fs-isolation branch 2 times, most recently from 958b513 to 3a4ad13 Compare July 21, 2021 06:17
src/shims/posix/fs.rs Outdated Show resolved Hide resolved
This allows catching extremely incorrect arguments before rejecting
due to isolation.
@RalfJung
Copy link
Member

Awesome, thanks a lot. :)
@bors r+

@bors
Copy link
Collaborator

bors commented Jul 25, 2021

📌 Commit 20d0f2e has been approved by RalfJung

@bors
Copy link
Collaborator

bors commented Jul 25, 2021

⌛ Testing commit 20d0f2e with merge 1677946...

@bors
Copy link
Collaborator

bors commented Jul 25, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 1677946 to master...

@bors bors merged commit 1677946 into rust-lang:master Jul 25, 2021
@atsmtat atsmtat deleted the fs-isolation branch July 25, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants