More careful permissions handling #4285
Conversation
The previous implementation would clobber anything Rust calls `permissions` that aren't visible to Unix chmod (for example, `metadata.permissions().readonly()`). Replaces with the method recommended at https://doc.rust-lang.org/std/os/unix/fs/trait.PermissionsExt.html#tymethod.set_mode.
* denoland/master: use Object instead of Map for promise table (denoland#4309) reorg: move js runtime tests to cli/js/tests/ (denoland#4250) upgrade: dprint 0.8.0 (denoland#4308) reorg: move JS ops implementations to cli/js/ops/, part 3 (denoland#4302) test: add actual error class to fail message (denoland#4305) upgrade: typescript 3.8.3 (denoland#4301) reorg: move JS ops implementations to cli/js/ops/, part 2 (denoland#4283) feat(std/node) add appendFile and appendFileSync (denoland#4294) disable test_raw_tty (denoland#4282)
|
This still needs a review. |
| let metadata = file.metadata()?; | ||
| let mut permissions = metadata.permissions(); | ||
| permissions.set_mode(mode); | ||
| file.set_permissions(permissions) |
There was a problem hiding this comment.
Why is this better? It looks like might make more syscalls.
If it's fixing a bug, there should be a test. If not, it shouldn't be changed.
There was a problem hiding this comment.
As I understand the Rust docs, the previous behavior was buggy. If a file was marked as "read-only" (this is exposed in the platform-neutral std::fs API, but I'm not sure what it amounts to on Unix, and I have minimal Windows experience,...) and it's run through the previous implementation of this function, then those read-only bits get erased and instead the mode bits are written to the file with no additional metadata. Same if the file has any metadata that the Rust API doesn't now expose. However, we can't make a test for this in TypeScript because we don't now have an API for creating or querying such additional metadata.
There was a problem hiding this comment.
Check out std::fs::Permissions and std::os::unix::fs::PermissionsExt. The old method creates a new permissions structure containing only the desired mode bits. The new method proposed here (and at the second link) reads the existing permissions structure, overwrites just its mode bits with the desired new ones, and then writes that permissions structure back to the file.
There was a problem hiding this comment.
I don't think there's a bug here. This isn't an op, just an internal function so it can be tested with a Rust unit test. If you can demonstrate a bug I would happily accept it. As this is now, as far as I can see, it's just moving code around. Thank you for the effort but I'm closing this PR without merge.
There was a problem hiding this comment.
Right, forgot we can make a Rust unit test. I'll see if I can demonstrate a bug doing that.
There was a problem hiding this comment.
@ry, it looks like you were correct. On Unix, the permissions().readonly/set_readonly only affect the chmod bits, and on Windows, the set_permissions function is a noop anyway, so it doesn't interfere with any readonly bits that may have been externally applied to a file. So the "fix" I proposed was unnecessary. Sorry for the noise.
This is part of a series of PRs towards #4017. It integrates commits from an earlier PR #4228.
This PR sets file permissions more hygienically in cli/fs.rs, and makes sure that the
modeargument to op_chmod is masked by 0o777. It also improvesdebug!calls in the associated functions.