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

Avoid TIOCSTI on Bash #416

Merged
merged 4 commits into from
Jun 1, 2024
Merged

Avoid TIOCSTI on Bash #416

merged 4 commits into from
Jun 1, 2024

Conversation

jtschuster
Copy link
Collaborator

To avoid using TIOCSTI on bash, mcfly can utilize the readline bindings. However, bind only lets you bind either a bash command / function or a readline function, but we can bind Ctrl+R to two keybindings - one which runs mcfly and binds/unbinds the second key to the accept-line readline depending on if enter or tab is pressed. It's a bit of a hack, so there's an escape hatch if a user prefers to use TIOCSTI.

@jtschuster jtschuster requested a review from cantino May 21, 2024 03:46
@cantino
Copy link
Owner

cantino commented May 21, 2024

Fascinating approach, I'll have to play with this and get back to you, thank you @jtschuster!

@cantino
Copy link
Owner

cantino commented May 25, 2024

It seems to basically work in bash on Mac and Linux. On Linux I am getting the following error during script setup: bash: [: =: unary operator expected

@jtschuster
Copy link
Collaborator Author

Thanks for the heads up, missed that in my testing. Looks like it was an issue with using single brackets in the if statements, should be fixed now.

@nuke-web3
Copy link

In #404 running cargo r -r -- -d search git for this branch brings up the TUI to select history correctly, but with sudo sysctl -w dev.tty.legacy_tiocsti=0 set now tab does not fill the command in the tty. (same behavior as before, I have not debuged if it's also from the same panic)

The same code referencing TIOCSTI exists in this branch that was panicing, so I am not sure this is a complete fix?

mcfly/src/fake_typer.rs

Lines 10 to 19 in caad816

#[cfg(not(windows))]
#[allow(clippy::useless_conversion)]
pub fn use_tiocsti(string: &str) {
for byte in string.as_bytes() {
let a: *const u8 = byte;
if unsafe { ioctl(0, libc::TIOCSTI.try_into().unwrap(), a) } < 0 {
panic!("Error encountered when calling ioctl");
}
}
}

@cantino
Copy link
Owner

cantino commented May 27, 2024

@nuke-web3 this will only work if executed from bash using the key bindings, I think.

@jamedeus
Copy link

Works on Ubuntu 24.04 server and desktop (bash 5.2.21, kernel 6.8.0-31)

@cantino
Copy link
Owner

cantino commented Jun 1, 2024

Works for me in linux and mac bash.

@jtschuster do you think there's any avenue for fixing it for manual executions of mcfly search? I suspect no.

@jtschuster
Copy link
Collaborator Author

No, I couldn't find any drop-in replacement for it.

@cantino cantino merged commit c70dc24 into cantino:master Jun 1, 2024
19 checks passed
@cantino
Copy link
Owner

cantino commented Jun 1, 2024

This is a good solution, thank you @jtschuster!

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.

4 participants