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

Use fork and exec for the command #468

Merged
merged 4 commits into from
Jun 17, 2023
Merged

Use fork and exec for the command #468

merged 4 commits into from
Jun 17, 2023

Conversation

pvdrz
Copy link
Collaborator

@pvdrz pvdrz commented Jun 15, 2023

Describe the changes done on this pull request

This PR uses fork and Command::exec to run the command instead of using Command::spawn. This is done because we need to set the command's process group as the foreground process group for the terminal and we cannot execute the command until it is set.

We cannot use Command::pre_exec to wait for the foreground process group to be set before executing the command because the monitor process will get stuck as Command::spawn will block until all the Command::pre_exec closures are executed.

This PR also adds the new concept of "exit reason" for the event loop. The main difference between an exit and a break is that setting the loop to break will stop the loop after the current callback is done, meanwhile setting the loop to exit will stop the loop after all the callbacks for the events that have already been polled are done. Meaning that "exit" is the "nice" way to stop the event loop.

This is another chunk of #363 and it is blocked by #467.

Pull Request Checklist

  • I have read and accepted the code of conduct for this project.
  • I have tested, formatted and ran clippy over my changes.
  • I have commented and documented my changes.
  • This pull request will help fix issue sudo-rs ignores SIGTSTP signal #325 where a proper discussion about a solution has taken place.

@github-actions
Copy link

github-actions bot commented Jun 15, 2023

Number of dependencies and binary size impact report

Metric main PR #468 Delta
Direct dependencies 6 6 -
Total dependencies 13 13 -
Binary size 927.5 KiB 930.1 KiB +0.3%
Text size 536.2 KiB 540.2 KiB +0.7%
Dependencies diff
 └─ sudo [v0.1.0-alpha.1]
    ├─ env_logger [v0.9.3]
    |  └─ log [v0.4.17]
    |     └─ cfg-if [v1.0.0]
    ├─ glob [v0.3.1]
    ├─ libc [v0.2.144]
    ├─ log [v0.4.17]
    ├─ signal-hook [v0.3.15]
    |  ├─ libc [v0.2.144]
    |  ├─ signal-hook-registry [v1.4.1]
    |  |  └─ libc [v0.2.144]
    |  └─ cc [v1.0.79]
    └─ signal-hook-registry [v1.4.1]

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Patch coverage: 73.17% and project coverage change: +0.15 🎉

Comparison is base (95093c4) 82.77% compared to head (1e8b055) 82.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
+ Coverage   82.77%   82.93%   +0.15%     
==========================================
  Files          52       52              
  Lines        6852     6961     +109     
==========================================
+ Hits         5672     5773     +101     
- Misses       1180     1188       +8     
Impacted Files Coverage Δ
sudo/lib/exec/parent.rs 70.10% <51.61%> (-5.45%) ⬇️
sudo/lib/exec/monitor.rs 69.90% <76.13%> (+3.23%) ⬆️
sudo/lib/exec/event.rs 88.29% <80.48%> (-5.74%) ⬇️
sudo/lib/exec/backchannel.rs 96.36% <100.00%> (+0.03%) ⬆️

... and 5 files with indirect coverage changes

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

@pvdrz pvdrz force-pushed the pvdrz-fork-and-exec branch 2 times, most recently from 9d5f1bd to 21c7d8b Compare June 16, 2023 00:28
Copy link
Collaborator

@japaric japaric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

err
})?;

let command_pid = command.id() as ProcessId;
if command_pid == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pattern comes up often when fork-ing. perhaps the signature of fork could be changed to take a child closure then this if ret == 0 can be moved into the body of the fork function. not something that needs to be addressed in this PR; just an idle though

@@ -47,6 +54,9 @@ pub(super) fn exec_monitor(
err
})?;

// Use a pipe to get the IO error if `exec_command` fails.
let (mut errpipe_tx, errpipe_rx) = UnixStream::pair()?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idle thought: this binary serialization over a pipe comes up in other places. perhaps a bit of abstraction could help with the buffer sizes and ser/de steps, e.g. Pipe<T: BinSerDe>, impl BinSerDef for i32 { const NUM_BYTES: usize = 4; /* .. */, Pipe::write(val: &T), Pipe::read() -> io::Result<T>, etc.

@pvdrz pvdrz added this pull request to the merge queue Jun 17, 2023
Merged via the queue into main with commit 25cc1e6 Jun 17, 2023
This was referenced Jun 19, 2023
@japaric japaric deleted the pvdrz-fork-and-exec branch June 19, 2023 11:14
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.

2 participants