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

docs: Permissions.readonly() also ignores root user special permissions #130289

Merged
merged 1 commit into from
Dec 23, 2024
Merged

docs: Permissions.readonly() also ignores root user special permissions #130289

merged 1 commit into from
Dec 23, 2024

Conversation

intgr
Copy link
Contributor

@intgr intgr commented Sep 12, 2024

The root user can write to files without any (write) permission bits set. But this is not taken into account by std::fs::Permissions.readonly().

The rustdoc for readonly() also mentions shortcomings later:

On Unix-based platforms this checks if any of the owner, group or others write permission bits are set. It does not check if the current user is in the file’s assigned group. It also does not check ACLs.

But since this part already clarifies how it works -- it checks write permission bits -- I think it's not necessary to repeat the root user shortcomings here.

@rustbot
Copy link
Collaborator

rustbot commented Sep 12, 2024

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 12, 2024
@ChrisDenton
Copy link
Member

I'm not really sure about this. I agree it's technically redundant but we've been really keen to emphasise the shortcomings of readonly/set_readonly as it can be a footgun, especially to those who aren't super familiar with the posix permissions model.

maybe @the8472 has some thoughts here?

@the8472
Copy link
Member

the8472 commented Oct 16, 2024

I think it'd be better to make this a broader statement that says that it only looks at a very limited set of properties of the file metadata and is not an accurate "will this file be non-writable" test and then refer to the platform-specific notes for more details.

E.g. on unix you can also create a file as read-only but have a writable file descriptor open.

@the8472
Copy link
Member

the8472 commented Oct 17, 2024

that it only looks at a very limited set of properties of the file metadata and is not an accurate

And "not accurate" can be expanded to "has both false positives and false negatives".

@Dylan-DPC
Copy link
Member

@intgr @the8472 what's the status of this? thanks

@intgr
Copy link
Contributor Author

intgr commented Nov 20, 2024

I'm sorry, I have somehow missed GitHub notifications about this PR.

It makes sense to make the 'Note' section shorter and explain this caveat in the 'Unix' section instead. Will change.

@intgr intgr force-pushed the Permissions-readonly-vs-unix-root branch from afa1bd2 to df9e232 Compare November 22, 2024 22:39
@intgr
Copy link
Contributor Author

intgr commented Nov 22, 2024

I have pushed changes:

  • I moved the general statement up into the Note section: "cannot be relied upon to predict whether attempts to read or write the file will actually succeed"
  • Moved the note about root user down into the Unix section

@intgr intgr force-pushed the Permissions-readonly-vs-unix-root branch from df9e232 to 9318a23 Compare December 2, 2024 21:17
ChrisDenton
ChrisDenton reviewed Dec 20, 2024
View reviewed changes
library/std/src/fs.rs Outdated Show resolved Hide resolved
@intgr
docs: Permissions.readonly() also ignores root user special permissions
edfdfbe 
The root user can write to files without any (write) access bits set. But this is not taken into account by `std::fs::Permissions.readonly()`.
@intgr intgr force-pushed the Permissions-readonly-vs-unix-root branch from 9318a23 to edfdfbe Compare December 22, 2024 18:47
@ChrisDenton
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 22, 2024

📌 Commit edfdfbe has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 22, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2024
@bors
Auto merge of rust-lang#134666 - matthiaskrgr:rollup-whe0chp, r=matth…
bd90c2a 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#130289 (docs: Permissions.readonly() also ignores root user special permissions)
 - rust-lang#134583 (docs: `transmute<&mut T, &mut MaybeUninit<T>>` is unsound when exposed to safe code)
 - rust-lang#134611 (Align `{i686,x86_64}-win7-windows-msvc` to their parent targets)
 - rust-lang#134629 (compiletest: Allow using a specific debugger when running debuginfo tests)
 - rust-lang#134642 (Implement `PointerLike` for `isize`, `NonNull`, `Cell`, `UnsafeCell`, and `SyncUnsafeCell`.)
 - rust-lang#134660 (Fix spacing of markdown code block fences in compiler rustdoc)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2024
@bors
Auto merge of rust-lang#134666 - matthiaskrgr:rollup-whe0chp, r=matth…
908af5b 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#130289 (docs: Permissions.readonly() also ignores root user special permissions)
 - rust-lang#134583 (docs: `transmute<&mut T, &mut MaybeUninit<T>>` is unsound when exposed to safe code)
 - rust-lang#134611 (Align `{i686,x86_64}-win7-windows-msvc` to their parent targets)
 - rust-lang#134629 (compiletest: Allow using a specific debugger when running debuginfo tests)
 - rust-lang#134642 (Implement `PointerLike` for `isize`, `NonNull`, `Cell`, `UnsafeCell`, and `SyncUnsafeCell`.)
 - rust-lang#134660 (Fix spacing of markdown code block fences in compiler rustdoc)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bd160f1 into rust-lang:master Dec 23, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2024
@rust-timer
Unrolled build for rust-lang#130289
5133b11 
Rollup merge of rust-lang#130289 - intgr-forks:Permissions-readonly-vs-unix-root, r=ChrisDenton

docs: Permissions.readonly() also ignores root user special permissions

The root user can write to files without any (write) permission bits set. But this is not taken into account by `std::fs::Permissions.readonly()`.

The rustdoc for `readonly()` also mentions shortcomings later:

> On Unix-based platforms this checks if any of the owner, group or others write permission bits are set. It does not check if the current user is in the file’s assigned group. It also does not check ACLs.

But since this part already clarifies how it works -- it checks write permission bits -- I think it's not necessary to repeat the root user shortcomings here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisDenton ChrisDenton ChrisDenton left review comments

Assignees

@ChrisDenton ChrisDenton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.
6 participants
@intgr @rustbot @ChrisDenton @the8472 @Dylan-DPC @bors