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

Fix for lost clipboard contents (#5424) #5426

Merged

Conversation

gavynriebau
Copy link
Contributor

@gavynriebau gavynriebau commented Jan 6, 2023

Fixes the issue described in #5424

The code has been changed so that for unix type platforms a call is made to libc::setsid using the unix specific CommandExt pre_exec.

The setsid call happens just after fork has been called and before execve is called. This means it runs in the context of the child process.

The call is necessary to ensure that spawned child processes related to the clipboard (e.g. xclip) have a distinct process session ID and process group ID.

Without this change when the parent process exits, the child (i.e. xclip), can be erroneously killed leading to copy/paste breaking in some specific circumstances such as those detailed in #5424

@gavynriebau gavynriebau force-pushed the bugfix/clipboard-contents-lost branch from a7e43c5 to 19f531f Compare January 6, 2023 07:03
Comment on lines 288 to 297
if cfg!(not(any(windows, target_os = "wasm32", target_os = "macos"))) {
use std::os::unix::process::CommandExt;

unsafe {
command_mut = command_mut.pre_exec(|| match libc::setsid() {
-1 => Err(std::io::Error::last_os_error()),
_ => Ok(()),
});
}
}
Copy link
Member

Choose a reason for hiding this comment

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

setsid should be called on all unix systems:

Suggested change
if cfg!(not(any(windows, target_os = "wasm32", target_os = "macos"))) {
use std::os::unix::process::CommandExt;
unsafe {
command_mut = command_mut.pre_exec(|| match libc::setsid() {
-1 => Err(std::io::Error::last_os_error()),
_ => Ok(()),
});
}
}
#[cfg(unix)]
unsafe {
use std::os::unix::process::CommandExt;
command_mut = command_mut.pre_exec(|| {
if libc::setsid() == -1 {
return Err(io::Error::last_os_error());
}
});
}

For windows we might also want to pass .creation_flags(CREATE_NEW_PROCESS_GROUP)

Copy link
Contributor Author

@gavynriebau gavynriebau Jan 7, 2023

Choose a reason for hiding this comment

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

I've made the following changes:

  • Call setsid for all unix systems
  • Call .creation_flags(CREATE_NEW_PROCESS_GROUP) for windows

After making the changes I've realised the change to set CREATE_NEW_PROCESS_GROUP will never be executed as it is inside a mod provider that uses the config #[cfg(not(target_os = "windows"))].

Seeking your opinion but I am thinking of reverting back by one commit so that the PR will contain no code related to setting CREATE_NEW_PROCESS_GROUP for windows but keeps the change you suggested of calling setsid for all unix systems?
Removed changes related to setting process group for windows as they weren't relevant

Separate to this PR but I'm also considering doing a follow-up PR to cleanup the code a little, let me know if you think this would be valuable?
With the amount of conditional compilation attributes I found it slightly hard to follow what was going on andwas thinking of splitting the code out into separate files for each environment, e.g. a provider file for windows, macos, osc52, wasm, unix etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these changes are ready for another review now.

I'm not able to change how pre_exec is called to exactly as you suggested because it needs to return a Result<()> type.

@kirawi kirawi added C-bug Category: This is a bug A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 9, 2023
@@ -22,6 +22,7 @@ helix-lsp = { version = "0.6", path = "../helix-lsp" }
helix-dap = { version = "0.6", path = "../helix-dap" }
crossterm = { version = "0.25", optional = true }
helix-vcs = { version = "0.6", path = "../helix-vcs" }
libc = "0.2"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be listed under [target.'cfg(unix)'.dependencies? It probably doesn't make a difference

Copy link
Contributor Author

@gavynriebau gavynriebau Jan 13, 2023

Choose a reason for hiding this comment

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

Thanks, yes, you're right, it's a dependency that's, for now, only used on unix platforms. I've addressed this now.

@gavynriebau gavynriebau force-pushed the bugfix/clipboard-contents-lost branch 2 times, most recently from 76b0e25 to e2b0b33 Compare January 13, 2023 10:32
@archseer archseer merged commit cce1971 into helix-editor:master Jan 16, 2023
@gavynriebau gavynriebau deleted the bugfix/clipboard-contents-lost branch January 17, 2023 13:43
gibbz00 pushed a commit to gibbz00/helix that referenced this pull request Jan 17, 2023
* Fix for lost clipboard contents (helix-editor#5424)

* PR feedback: Call "setsid" for all unix systems

* PR Feedback: Only install libc for unix targets
kirawi pushed a commit to kirawi/helix that referenced this pull request Jan 25, 2023
* Fix for lost clipboard contents (helix-editor#5424)

* PR feedback: Call "setsid" for all unix systems

* PR Feedback: Only install libc for unix targets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants