Revive RFC: Change Process to use directory fd #171

sunfishcode · 2022-03-15T15:43:07Z

This updates and polishes the patch from #127, which seems to be abandoned, as the author is listed as @ghost. It uses openat and readlinkat to protect against PID reuse.

This also makes two additional changes: it adds a dependency on rustix, which provides safe APIs for openat, readlinkat, and so on, and it bumps the MSRV to 1.48. I know you just recently bumped the MSRV to 1.42, but 1.48 is worth considering, as it's the version of Rust currently in Debian stable, it's newer than the version in the oldest still-supported Fedora release, and it eliminates the need for special instructions for the hex and bitflags dependencies. That said, people have different needs, and of course it's your call as to whether these changes are appropriate for procfs.

Fixes #125.

All data accessors now use openat and readlinkat when reading procfs entries, to prevent PID reuse. Co-authored-by: Jason Francis <zxzax@protonmail.com>

eminence · 2022-03-16T01:04:49Z

Hey, this is pretty neat, thanks for the PR. There's a lot to unpack (and to review) here, so please bear with me...

I took a quick skim and despite the large diff, everything is pretty readable. But I do want to spend some more time looking at all the changes. A few brief comments for now:

I think I'm fine bumping the MSRV to 1.48, given what you said about the versions currently shipping in Debian and Fedora (thanks for those details). 1.48 is already more than 1 year old, and if anyone really needs to use something older, I'm OK asking them to stay on v0.12 (if merged, this PR would be part of a v0.13 release).
The change to use openat and friends looks nice. In particular, the simplification of Process::is_alive is a good demonstration of the benefits of this approach, I like it.
I'm not opposed to the port to rustix, but I do want to study it a bit more. In particular, a small number of rustix types are exposed as part of the public API of this crate, so the port to rustix is not quite an entirely internal implementation details.

These are implementation details that don't need to be part of the public API.

This avoids exposing `rustix::fs::Stat` in the public API, and makes the main obvious use case simpler.

This hides the use of `BorrowedFd` to hold a procfs directory file descriptor from the public API.

This `impl` is visible in the public API, so hide it, and just manually convert error types as needed.

This takes the rustix `RawMode` type out of the public API, and it uses fewer bits.

sunfishcode · 2022-03-16T02:37:45Z

* I'm not opposed to the port to `rustix`, but I do want to study it a bit more.  In particular, a small number of `rustix` types are exposed as part of the public API of this crate, so the port to `rustix` is not quite an entirely internal implementation details.

Some of those things were APIs which don't actually need to be public, and others were things that could be avoided. I've now fixed them, so rustix doesn't appear in the public API at all.

eminence · 2022-03-16T03:33:19Z

Thanks, I appreciate that

src/net.rs

src/sys/fs/binfmt_misc.rs

src/process/stat.rs

src/process/task.rs

src/process/mod.rs

…ific.

sunfishcode · 2022-04-04T12:23:21Z

Is there anything else you'd like to see in this PR?

eminence · 2022-04-04T13:05:30Z

No, sorry for my delinquency here. I did spend some time trying to update a project to using this new version, and I will say that the fact that Process is no longer clone and the fact that there is no longer a pre-populated stat field was surprisingly annoying. I think I'm going to want to think about a new quality-of-life APIs before releasing this.

But that of course doesn't need to help up this PR.

One final question for you: How closely should I be following rustix versions? Do you think the current minimum version of 0.34.0 will be fine for the foreseeable future?

sunfishcode · 2022-04-04T13:54:07Z

Yeah, 0.34.0 should be good for a while. There will likely be a 0.35 at some point over the next few months, to synchronize rustix with the io_safety API in Rust as it stabilizes, but I'm expecting to backport any important bug fixes to 0.34 point releases.

src/process/mod.rs

It apppears I accidentally removed `all_processes_with_root` in eminence#171. This PR reintroduces it.

This fixes a regression that was introduced in #171 Closes #197

This fixes a regression that was introduced in #171 Closes #197 (cherry picked from commit 20b1182)

This fixes a regression that was introduced in eminence#171 Closes eminence#197

sunfishcode and others added 5 commits March 15, 2022 08:01

Update the MSRV to 1.48.

c69bb94

Fix clippy lints, now that MSRV is 1.48.

c97b390

Fix more clippy lints.

bd1651f

Port to rustix.

d23fe24

Change Process to use directory fd

804717f

All data accessors now use openat and readlinkat when reading procfs entries, to prevent PID reuse. Co-authored-by: Jason Francis <zxzax@protonmail.com>

sunfishcode added 6 commits March 15, 2022 18:19

Make the fd members of Process and Task private.

79515e4

These are implementation details that don't need to be part of the public API.

Replace Process::metadata with Process::uid in the public API.

5fdcb2e

This avoids exposing `rustix::fs::Stat` in the public API, and makes the main obvious use case simpler.

Make FDInfo::from_process_at private.

7ff9a62

This hides the use of `BorrowedFd` to hold a procfs directory file descriptor from the public API.

Remove the impl From<rustix::io::Error> for ProcError.

afeeb30

This `impl` is visible in the public API, so hide it, and just manually convert error types as needed.

Represent FDPermissions as a u16.

211c0d6

This takes the rustix `RawMode` type out of the public API, and it uses fewer bits.

Update to rustix 0.34.0.

3bfcc25

eminence approved these changes Mar 17, 2022

View reviewed changes

src/net.rs Outdated Show resolved Hide resolved

src/sys/fs/binfmt_misc.rs Show resolved Hide resolved

src/process/stat.rs Show resolved Hide resolved

src/process/task.rs Outdated Show resolved Hide resolved

src/process/mod.rs Outdated Show resolved Hide resolved

sunfishcode added 3 commits March 16, 2022 20:45

Revert a utime threshold change.

7485ad9

Delete a redundant comment, and make the remaining commment more spec…

92601b4

…ific.

Simplify code by just using OwnedFd and not bothering with File.

93fe550

eminence mentioned this pull request Mar 18, 2022

Improve CpuInfo performances #168

Merged

eminence merged commit 8c4949c into eminence:master Apr 5, 2022

sunfishcode mentioned this pull request Apr 5, 2022

RFC: Change Process to use directory fd #127

Closed

sunfishcode deleted the sunfishcode/rustix branch April 5, 2022 11:37

eminence reviewed Jul 11, 2022

View reviewed changes

src/process/mod.rs Show resolved Hide resolved

sunfishcode added a commit to sunfishcode/procfs that referenced this pull request Jul 11, 2022

Re-introduce all_processes_with_root.

37c44c6

It apppears I accidentally removed `all_processes_with_root` in eminence#171. This PR reintroduces it.

sunfishcode mentioned this pull request Jul 11, 2022

Re-introduce all_processes_with_root. #184

Merged

obaudys mentioned this pull request Jul 22, 2022

procfs::process::all_processes() can blow "open files" ulimit on a system with a large number of processes #188

Closed

eminence added a commit that referenced this pull request Aug 20, 2022

Don't consider Zombies to be alive

20b1182

This fixes a regression that was introduced in #171 Closes #197

eminence mentioned this pull request Aug 20, 2022

Don't consider Zombies to be alive #198

Merged

eminence added a commit that referenced this pull request Aug 20, 2022

Don't consider Zombies to be alive

94457d0

This fixes a regression that was introduced in #171 Closes #197 (cherry picked from commit 20b1182)

eminence added a commit that referenced this pull request Aug 20, 2022

Don't consider Zombies to be alive

84c6c24

This fixes a regression that was introduced in #171 Closes #197 (cherry picked from commit 20b1182)

eminence added a commit that referenced this pull request Aug 21, 2022

Don't consider Zombies to be alive

f8356f0

This fixes a regression that was introduced in #171 Closes #197 (cherry picked from commit 20b1182)

ludo-c pushed a commit to ludo-c/procfs that referenced this pull request Sep 18, 2022

Don't consider Zombies to be alive

4d8521c

This fixes a regression that was introduced in eminence#171 Closes eminence#197

ludo-c pushed a commit to ludo-c/procfs that referenced this pull request Oct 7, 2022

Don't consider Zombies to be alive

d71b9f5

This fixes a regression that was introduced in eminence#171 Closes eminence#197

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revive RFC: Change Process to use directory fd #171

Revive RFC: Change Process to use directory fd #171

sunfishcode commented Mar 15, 2022

eminence commented Mar 16, 2022

sunfishcode commented Mar 16, 2022

eminence commented Mar 16, 2022

sunfishcode commented Apr 4, 2022

eminence commented Apr 4, 2022

sunfishcode commented Apr 4, 2022

Revive RFC: Change Process to use directory fd #171

Revive RFC: Change Process to use directory fd #171

Conversation

sunfishcode commented Mar 15, 2022

eminence commented Mar 16, 2022

sunfishcode commented Mar 16, 2022

eminence commented Mar 16, 2022

sunfishcode commented Apr 4, 2022

eminence commented Apr 4, 2022

sunfishcode commented Apr 4, 2022