trifectatechfoundation / sudo-rs

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

Handle `SIGWINCH` with `use_pty` enabled #649

Closed pvdrz closed 1 year ago

pvdrz commented 1 year ago

Describe the changes done on this pull request This PR updates the use_pty behavior so it can handle SIGWINCH correctly.

I couldn't find a way to write a compliance test for this as stty doesn't send SIGWINCH when used to update the terminal size.

Pull Request Checklist

github-actions[bot] commented 1 year ago

Number of dependencies and binary size impact report

Metric main PR #649 Delta
Direct dependencies 3 3 -
Total dependencies 4 4 -
Binary size 978.4 KiB 983.6 KiB +0.5%
Text size 570.1 KiB 572.6 KiB +0.4%
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 has no change and project coverage change: -0.36 :warning:

Comparison is base (4f7c03f) 59.50% compared to head (af8af21) 59.15%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #649 +/- ## ========================================== - Coverage 59.50% 59.15% -0.36% ========================================== Files 66 66 Lines 8781 8830 +49 ========================================== - Hits 5225 5223 -2 - Misses 3556 3607 +51 ``` | [Impacted Files](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) | Coverage Δ | | |---|---|---| | [src/exec/no\_pty.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2V4ZWMvbm9fcHR5LnJz) | `0.00% <0.00%> (ø)` | | | [src/exec/use\_pty/parent.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2V4ZWMvdXNlX3B0eS9wYXJlbnQucnM=) | `0.00% <0.00%> (ø)` | | | [src/system/term/mod.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL3N5c3RlbS90ZXJtL21vZC5ycw==) | `63.90% <0.00%> (-8.13%)` | :arrow_down: | | [src/system/term/user\_term.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL3N5c3RlbS90ZXJtL3VzZXJfdGVybS5ycw==) | `0.00% <0.00%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/649/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.

japaric commented 1 year ago

I couldn't find a way to write a compliance test for this as stty doesn't send SIGWINCH when used to update the terminal size.

you can send SIGWINCH to a process using kill. even if you can't actually resize the terminal, you can at least test that the ioctl calls don't fail and that the signal gets forwarded to the child

pvdrz commented 1 year ago

I couldn't find a way to write a compliance test for this as stty doesn't send SIGWINCH when used to update the terminal size.

you can send SIGWINCH to a process using kill. even if you can't actually resize the terminal, you can at least test that the ioctl calls don't fail and that the signal gets forwarded to the child

Apparently the shell notices this change

I couldn't find a way to write a compliance test for this as stty doesn't send SIGWINCH when used to update the terminal size.

you can send SIGWINCH to a process using kill. even if you can't actually resize the terminal, you can at least test that the ioctl calls don't fail and that the signal gets forwarded to the child

Apparently the shell sends SIGWINCH to the foreground process group if you change the size with stty. So if you open a terminal and run

# to get the terminal path
tty 
sh -c "trap \"echo GOT SIGWINCH\" WINCH; sleep 20"

and then, from another terminal you run

stty -F/dev/pts/N cols 42

You'll see the GOT SIGWINCH message in the first terminal after 20 secs