kill: implementation for Windows - #13604
Conversation
Merging this PR will degrade performance by 4.47%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | hostname_ip_lookup[100000] |
109.2 µs | 119.8 µs | -8.81% |
| ❌ | Memory | factor_multiple_u64s[2] |
54.1 KB | 57.6 KB | -6.13% |
| ❌ | Memory | hostname_basic |
56.3 KB | 59.8 KB | -5.9% |
| ❌ | Memory | seq_custom_separator |
58.4 KB | 61.9 KB | -5.7% |
| ❌ | Memory | wc_lines_extreme_line_lengths[(100000, 200)] |
65.1 KB | 68.6 KB | -5.14% |
| ❌ | Memory | nl_many_lines[100000] |
66.2 KB | 69.7 KB | -5.06% |
| ❌ | Memory | unexpand_many_lines[100000] |
67 KB | 70.5 KB | -5.01% |
| ❌ | Memory | expand_custom_tabstops[50000] |
67 KB | 70.5 KB | -5% |
| ❌ | Memory | cut_fields_newline_delim |
69.3 KB | 72.8 KB | -4.85% |
| ❌ | Memory | fold_custom_width[50000] |
76.1 KB | 79.6 KB | -4.43% |
| ❌ | Memory | split_numeric_suffix |
80.5 KB | 84 KB | -4.2% |
| ❌ | Memory | df_deep_directory |
80.7 KB | 84.3 KB | -4.19% |
| ❌ | Memory | join_custom_separator |
86 KB | 89.5 KB | -3.94% |
| ❌ | Memory | cksum_multiple_files |
91.2 KB | 94.7 KB | -3.73% |
| ❌ | Memory | rm_recursive_tree |
98 KB | 101.5 KB | -3.48% |
| ❌ | Simulation | single_date_now |
83.8 µs | 86.6 µs | -3.22% |
| ⚡ | Simulation | wc_bytes_synthetic[1] |
83.9 µs | 81.2 µs | +3.25% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing nikolalukovic:feature/kill-windows (9dcb1d0) with main (41059cf)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
GNU testsuite comparison: |
| } | ||
|
|
||
| /// Deliver `signal` (POSIX numbering) to the arbitrary process `pid`, | ||
| /// emulating `kill(2)` for `pid > 0`. `SeDebugPrivilege` is never enabled. |
There was a problem hiding this comment.
Why is SeDebugPrivilege never enabled?
There was a problem hiding this comment.
it would go against the goals of the project and POSIX, self-escalation is strictly forbidden and considering how the privilege model is wildly different between Windows and Unix this felt, to me, like going with the best behaviour match. we could go with a separate windows only flag to emulate taskkill /F but that feels like shifting away from the core goals of the project too much
There was a problem hiding this comment.
I'm not entirely sure I understand what you mean. Can enabling rights you already have access to really be considered self-escalation? No process on Windows just has SeDebugPrivilege automatically.
There was a problem hiding this comment.
oof I assumed this wasn't done but I just saw Cygwin does actually use sedebugprivilege in the force kill path, I completely misunderstood how it works. is it okay if we do the same here or should we just try and do it all the time without -f?
There was a problem hiding this comment.
I'd be fine trying to enable it unconditionally. We could always fall back if it was not allowed!
| _ => Err(unsupported(translate!( | ||
| "kill-error-process-groups-unsupported" | ||
| ))), |
There was a problem hiding this comment.
how do we get the handle?
There was a problem hiding this comment.
There's no direct public Win32 or NT API for that. So, we got to get creative, and creative is unfortunately a bit involved. If you're using AI to code this, I assume it won't trouble you too much. Otherwise, let's consider this a TODO. The idea would be to iterate through all processes using NT APIs and filter down to the current process and get its Job ID. Then get all handles with SystemExtendedHandleInformation and get the matching Job handle and kill it. Something like that - it's probably quite a bit of code. (More than I would've assumed intuitively. 🙁)
…heck in send_signal_to_pid
| and STOP are not supported. Permissions come from your current token: kill | ||
| never enables SeDebugPrivilege, so an elevated kill may report 'Permission |
There was a problem hiding this comment.
(Would you mind explaining in the PR body why not use SeDebugPrivilege?)
There was a problem hiding this comment.
I'm sorry, I missed the earlier comments as they were at the top level rather than in a file :)
There was a problem hiding this comment.
ye, I'm being educated left and right on this PR :D
5 participants
Implement kill util for Windows since #13363 added most of plumbing needed.