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.

Sign up for GitHub

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

Jump to bottom

On illumos tcsetattr() is unusably buggy as it truncates termios fields #2071

Open
nospam3089 opened this issue Jul 12, 2023 · 5 comments · May be fixed by #2253
Open

On illumos tcsetattr() is unusably buggy as it truncates termios fields #2071

nospam3089 opened this issue Jul 12, 2023 · 5 comments · May be fixed by #2253
Labels
A-bug

Comments

@nospam3089
Copy link

Attempting to run:

use nix::sys::termios::{tcgetattr, tcsetattr};

fn main() {
    let tty_in = std::io::stdin();
    let original_mode = tcgetattr(&tty_in).unwrap();
    println!("tcgetattr(): {original_mode:?}");
    println!("c_iflag: {}", libc::BRKINT | libc::IGNPAR | libc::ICRNL | libc::IXON |
        libc::IMAXBEL);
    println!("c_oflag: {}", libc::OPOST | libc::ONLCR);
    println!("c_cflag: {}", libc::CS6 | libc::CS7 | libc::CREAD);
    println!("c_lflag: {}", libc::ECHOKE | libc::ECHOE | libc::ECHOK | libc::ECHO | libc::ECHOCTL |
        libc::ISIG | libc::ICANON | libc::IEXTEN);

    std::thread::sleep(std::time::Duration::from_secs(10));
    tcsetattr(&tty_in, nix::sys::termios::SetArg::TCSADRAIN, &original_mode).unwrap();
    println!("Back from tcsetattr()?");
    std::thread::sleep(std::time::Duration::from_secs(10));
}

will output something like:

tcgetattr(): Termios { inner: RefCell { value: termios { c_iflag: 9478, c_oflag: 6149, c_cflag: 983231, c_lflag: 35387, c_cc: [3, 28, 127, 21, 4, 0, 0, 0, 17, 19, 26, 25, 18, 15, 23, 22, 20, 8, 0] } }, input_flags: InputFlags(BRKINT | IGNPAR | ICRNL | IXON | IMAXBEL), output_flags: OutputFlags(OPOST | ONLCR), control_flags: ControlFlags(CS6 | CS7 | CREAD), local_flags: LocalFlags(ECHOKE | ECHOE | ECHOK | ECHO | ECHOCTL | ISIG | ICANON | IEXTEN), control_chars: [3, 28, 127, 21, 4, 0, 0, 0, 17, 19, 26, 25, 18, 15, 23, 22, 20, 8, 0] }
c_iflag: 9478
c_oflag: 5
c_cflag: 176
c_lflag: 35387

Please notice two things:

  • The values of c_oflag and c_cflag have been truncated.
  • The terminal was hung up/terminated prior to the string Back from tcsetattr()? had any chance to get printed.

Quoting tcsetattr (3C):

The effect of tcsetattr() is undefined if the value of the termios structure pointed to by termios_p was not derived from the result of a call to tcgetattr(3C) on fildes; an application should modify only fields and flags defined by this document between the call to tcgetattr(3C) and tcsetattr(), leaving all other fields and flags unmodified.

There are also other relevant sections in the manual, but the bottom line is that tcsetattr() practically always close the terminal when called on illumos as currently implemented.

I can think of three ways to improve the current situation:

  • Make nix and libc aware of all flags on illumos.
  • Pass through the original values unmodified from their raw representation for all unknown bits.
  • Simply disable tcsetattr() on illumos.

Given the state of #935 and rust-lang/libc#1405, the first alternative does not seems likely to happen any time soon. The second alternative seems pragmatic, but would be a bit messy both to implement and maintain. The third alternative seems less desired than actually fixing the functionality.

I could do a PR to remove tcsetattr() (or rather the entire termios module?) on illumos, unless someone has a better feasible idea.
nospam3089 added a commit to nospam3089/rustyline that referenced this issue Jul 12, 2023
@nospam3089
Use termios from termios crate
a2c20a8 
The termios interface from the nix crate is lacking a thought through
design. Severely broken on at least one platform. Please see [#2071][]
for details.

The termios crate stays closer to the C API and thus both works with
illumos and reduces the risk of bugs on other platforms.

[#2071]: nix-rust/nix#2071
@nospam3089 nospam3089 mentioned this issue Jul 12, 2023
Merged
nospam3089 added a commit to nospam3089/rustyline that referenced this issue Jul 14, 2023
@nospam3089
Use termios from termios crate
c3a2d08 
The termios interface from the nix crate is lacking a thought through
design. Severely broken on at least one platform. Please see [#2071][]
for details.

The termios crate stays closer to the C API and thus both works with
illumos and reduces the risk of bugs on other platforms.

[#2071]: nix-rust/nix#2071
@asomers
Copy link
Member

asomers commented Jul 15, 2023

Adding missing flags to libc isn't very hard. It's much easier than adding illumos CI. So that's certainly the path of least resistance. It isn't future-proof, however, as it could be broken by future Illumos releases that add new flags.

Passing through the exact libc::termios value, potentially with modifications, would require more work. Instead of making Termios::input_flags pub for example, we might have to create Termios::{set_input_flags,clear_input_flags} methods that manipulate the raw structure while preserving unknown bits. It could be done, but I for one don't have the time to do it.

Disabling termios on illumos would be slightly sad, but only slightly. I would accept that if you really don't have time or ability to add the additional flags to libc.

@nospam3089
Copy link
Author

Thanks for your involved response and for actively maintaining this crate!

It seems we agree the best fix would be to adapt the methods to be gentle with unknown bits. Given the man page quote above, I believe that is the only way to make the API safe on illumos. Since making Termios::input_flags private would require a major version increase, it might be worth considering whether also other breaking changes are in queue?

Unfortunately my time for this is also a bit sparse, but I'll make an attempt at a PR during next week. Then we'll see if it becomes good enough for inclusion, or if we'll have to settle with the unsettling alternative of disabling illumos support.

@asomers
Copy link
Member

asomers commented Jul 17, 2023

Don't worry about breaking changes. Nix isn't 1.0 , so we can and do make those from time to time.

@nospam3089
Copy link
Author

I've pushed the two branches fix/disable_tcsetattr_illumos and fix/safe_termios, but they are not at all tested. The test suites seem to pass without adding any new failures, but I have no code to do sanely validate the functionality with.

Since I'm not a direct consumer of the nix crate, and the crate which made me encounter the problem has conflicting outdated dependencies, I'm afraid it seems to be a bit too much and too fiddly work for me to bring this to completion.

With the original C struct already stored in Termios, I believe the fix could actually be as simple as merely preserving the original flags as attempted on the fix/safe_termios branch, but I might need to leave this issue percolating until someone actually able to verify it happens to pass along.

Should I post the illumos-disabling branch to a PR, or what seems like the best way forward?

@asomers
Copy link
Member

asomers commented Aug 11, 2023

I am not an Illumos user, and I'm not an expert on the termios API either. If you don't have time to finish that branch, then we can't merge it. But if the current code is useless on Illumos, then sure we can merge a PR to disable it.

@sunfishcode sunfishcode mentioned this issue Oct 6, 2023
Closed
@asomers asomers linked a pull request Dec 8, 2023 that will close this issue
Open
3 tasks
@SteveLauC SteveLauC added the A-bug label Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use from_bits_retain for all Termios wrapper fields. qwandor/nix
3 participants
@asomers @nospam3089 @SteveLauC