trifectatechfoundation / sudo-rs

A memory safe implementation of sudo and su.
Other
2.9k stars 79 forks source link

Assert that sockets won't block only if they are polled beforehand #624

Closed pvdrz closed 1 year ago

pvdrz commented 1 year ago

Describe the changes done on this pull request This PR moves the assertions about the backchannel operations being not blocking to the event loop where the backchannel sockets are actually being polled.

Pull Request Checklist

github-actions[bot] commented 1 year ago

Number of dependencies and binary size impact report

Metric main PR #624 Delta
Direct dependencies 3 3 -
Total dependencies 4 4 -
Binary size 1001.6 KiB 1001.6 KiB -
Text size 580 KiB 580.2 KiB -
Dependencies diff ```diff └─ sudo-rs [v0.2.0-dev.20230703] ├─ glob [v0.3.1] ├─ libc [v0.2.147] └─ log [v0.4.19] ```
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 47.82% and project coverage change: -0.14 :warning:

Comparison is base (636351d) 86.51% compared to head (7122e28) 86.37%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #624 +/- ## ========================================== - Coverage 86.51% 86.37% -0.14% ========================================== Files 65 65 Lines 8674 8721 +47 ========================================== + Hits 7504 7533 +29 - Misses 1170 1188 +18 ``` | [Impacted Files](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/624?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) | Coverage Δ | | |---|---|---| | [src/exec/use\_pty/parent.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/624?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2V4ZWMvdXNlX3B0eS9wYXJlbnQucnM=) | `73.37% <30.00%> (+0.20%)` | :arrow_up: | | [src/exec/use\_pty/monitor.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/624?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2V4ZWMvdXNlX3B0eS9tb25pdG9yLnJz) | `52.36% <45.45%> (+0.30%)` | :arrow_up: | | [src/exec/use\_pty/backchannel.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/624?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2V4ZWMvdXNlX3B0eS9iYWNrY2hhbm5lbC5ycw==) | `76.87% <57.89%> (-3.60%)` | :arrow_down: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/624/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

pvdrz commented 1 year ago

mmm to be honest I think blocking is fine for all the cases, even when we don't poll the sockets. Because we actually want to block processes if we're waiting for stuff to arrive in the sockets. Like the "green light" message, or the "process PID" message.

But I agree that having the assertions inside Backchannel is a better idea. So what about having two methods that just enable/disable the assertions instead: enable_non_blocking_assertions and disable_non_blocking_assertions?

japaric commented 1 year ago

I think my main concern is that, even without the assertions, if the sockets are set to nonblocking mode right after they are open and a blocking operation happens (e.g. the green light message) then any use of ? could make a read / write early return and e.g. take down the monitor or any other process.

so, we should move that set_nonblocking to right before we after using poll. if we are going to add a enable_non_blocking_assertions method then we may as well move the set_nonblocking call into it -- and enable_non_blocking_assetions should be called after 'green light' and before the poll loop.

pvdrz commented 1 year ago

mmm yeah I get your point.

The good thing is that we "busywait" in those cases where using the socket could fail due to EINTR or EAGAIN. So, the green light recv call will be retried until we get an error that's not Interrupted or WouldBlock.