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

Work around missing TCGETS2/TCSETS2 on WSL and Android. #1146

Closed
wants to merge 2 commits into from

Conversation

sunfishcode
Copy link
Member

WSL appears to be lacking support for TCGETS2 and TCSETS2, so teach rustix's tcgetattr and tcsetattr how to fall back to TCGETS and TCSETS as needed.

This approach preserves rustix's ability to support arbitrary speed values, while falling back as needed to support WSL.

This is expected to fix crossterm-rs/crossterm#912.

WSL appears to be lacking support for `TCGETS2` and `TCSETS2`, so teach
rustix's `tcgetattr` and `tcsetattr` how to fall back to `TCGETS` and
`TCSETS` as needed.

This approach preserves rustix's ability to support arbitrary speed values,
while falling back as needed to support WSL.

This is expected to fix crossterm-rs/crossterm#912.
@joshka
Copy link

joshka commented Aug 31, 2024

This implementation adds a fallback approach, which tries to use the TCGETS/SETS2 and when that doesn't work tries TCGETS/SETS. This introduces state to a fundamental call in the library, which can make it a bit more difficult to reason about and maintain. My spidey senses suggest this may not be desirable (purely from a low level API design, however this is coming from someone not too familiar with Rustix, so take this with a grain of salt).

Another possible approach to consider is to use a compile time flag for the affected operating systems (WSL and Android seem to have problems as reported by users of Ratatui / Crossterm which calls this method eventually). Is there a way to disambiguate WSL from Linux here, or to otherwise detect the availability of these methods at build time rather than runtime?

I'm also curious if there's some missing CI configuration that would help show that the call failure on WSL/Android (and which would show that it's fixed by this code change). This would also generally help highlight any other missing syscalls.

@sunfishcode
Copy link
Member Author

sunfishcode commented Aug 31, 2024

This implementation adds a fallback approach, which tries to use the TCGETS/SETS2 and when that doesn't work tries TCGETS/SETS. This introduces state to a fundamental call in the library, which can make it a bit more difficult to reason about and maintain. My spidey senses suggest this may not be desirable (purely from a low level API design, however this is coming from someone not too familiar with Rustix, so take this with a grain of salt).

I agree, it isn't great.

Another possible approach to consider is to use a compile time flag for the affected operating systems (WSL and Android seem to have problems as reported by users of Ratatui / Crossterm which calls this method eventually). Is there a way to disambiguate WSL from Linux here, or to otherwise detect the availability of these methods at build time rather than runtime?

I haven't seen reports of Android failing; do you have any links to such reports?

As I understand it, the designers of WSL deliberately chose for it to not be a separate target or build configuration, so there is no compile-time or build-time check we could do.

From some quick searching it looks like we could probably do a runtime check for WSL using uname and looking for the string "microsoft". However, that probably also would want to store state in a static variable so that we don't keep calling uname every time.

I'm also curious if there's some missing CI configuration that would help show that the call failure on WSL/Android (and which would show that it's fixed by this code change). This would also generally help highlight any other missing syscalls.

We have a confirmation here that the crossterm issue is fixed by this code change.

I'm not familiar enough with WSL to be able to set up a CI configuration for it at this time. I would welcome anyone volunteering to help set this up.


There is another possible solution, which is to just make tcgetattr use TCGETS/TCSETS instead of using TCGETS2/TCSETS2. The only difference is that TCGETS2/TCSETS2 support for the c_ispeed and c_ospeed fields, which add support for arbitrary speeds. Speeds mainly apply to serial ports, and don't apply to virtual terminals, so most users don't need them.

I decided to use TCGETS2/TCSETS2 in rustix originally because they've been supported in Linux since 2.6.20, and it means we can have a single tcgetattr API that works for both virtual terminals and serial ports. The alternative seems to be to have tcgetattr use TCGETS and add a new tcgetattr_serialport function uses TCGETS2. Perhaps in light of this WSL bug, it's worth considering doing this now. But if we do, we'll need to consider the risk of breaking existing users.

@joshka
Copy link

joshka commented Aug 31, 2024

Some added contex on above, I assumed incorrectly that you were one of the devs impacted by the crossterm issue and not the rustix maintainer. Apologies for my confusion. You're very definitely more in the know about what's appropriate for rustix than me.

I haven't seen reports of Android failing; do you have any links to such reports?

One of the reports of this problem was on the ratatui discord (context - I'm a Ratatui maintainer), I think this was either the same issue or a very closely related one. https://discord.com/channels/1070692720437383208/1072907135664529508/1278272183470264362
I can't easily paste that here unfortunately and I'm not sure what the user's github username is to ping them.

Regarding how this impacts Ratatui, all our examples etc. use crossterm, and the latest Ratatui release has also bumped to the latest Crossterm release. I also (re)wrote the PR in crossterm to use rustix instead of libc (it was a rewrite of another PR which did a hard switch into a PR which did a feature flag gated switch). crossterm-rs/crossterm#892

I wonder if something to consider here is build time logic in build.rs. Could build.rs detect whether the target environment supports the syscall and compile differently based on the answer, or does that cause other problems.

Regarding WSL in CI, I too don't know much there. I've been Mac only since before WSL existed, so I have zero direct experience with it.

@sunfishcode sunfishcode changed the title Work around missing TCGETS2/TCSETS2 on WSL. Work around missing TCGETS2/TCSETS2 on WSL and Android. Aug 31, 2024
@sunfishcode
Copy link
Member Author

Thanks for that Android link. It appears Android fails in a similar but slightly different way; I've now updated the PR to handle that. And beyond Android, it's possible other seccomp users could potentially hit this as well.

@sunfishcode
Copy link
Member Author

I wonder if something to consider here is build time logic in build.rs. Could build.rs detect whether the target environment supports the syscall and compile differently based on the answer, or does that cause other problems.

I'd prefer such an approach in general, however since WSL is meant to run regular Linux binaries, I suspect anything we do here wouldn't work in some situations.

@joshka
Copy link

joshka commented Sep 1, 2024

Digging a bit deeper, it seems that glibc doesn't support TCGETS2 at all (there's a 7 year old bugzilla bug), neither does freebsd's libc or musl (I checked the sources). Would this suggest that the right approach here would be to do the same and only use TCGETS here?

@sunfishcode
Copy link
Member Author

Digging a bit deeper, it seems that glibc doesn't support TCGETS2 at all (there's a 7 year old bugzilla bug), neither does freebsd's libc or musl (I checked the sources). Would this suggest that the right approach here would be to do the same and only use TCGETS here?

On FreeBSD and macOS and all BSD-family platforms, the regular tcgetattr ioctl just supports arbitrary speeds. That's one of the reasons I chose to use TCGETS2 on Linux in the first place; that's how tcgetattr "should" just work.

The glibc isuse discusses how glibc really should switch to TCGETS2, however it doesn't seem to have been finished. Musl recently discussed it here and seems to have prioritized glibc compatibility.

Another option would be to have tcgetattr start with TCGETS and if that sets the BOTHER flag then and only then fall back to TCGETS2. tcsetattr can just check whether BOTHER is set and pick between TCSETS and TCSETS2 accordingly. That would avoid the need to change rustix's public API, and would be slightly simpler, at the cost of being a little less optimistic and less efficient for serial-port users.

@joshka
Copy link

joshka commented Sep 1, 2024

Sounds reasonable to me.

@sunfishcode
Copy link
Member Author

Investigating more, it appears there are more seccomp configurations out there which only recognize TCGETS and not TCGETS2, because glibc/musl don't use TCGETS2 so people never see it so they never add it to their sandbox rules. So it seems best for rustix to stick to TCGETS unless it detects a need for TCGETS2.

I've now filed #1147 to implement this alternative approach.

@sunfishcode
Copy link
Member Author

Closing in favor of #1147.

@sunfishcode sunfishcode closed this Sep 5, 2024
@sunfishcode sunfishcode deleted the sunfishcode/tcgets2-wsl branch September 5, 2024 12:40
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.

enable_raw_mode() error in version 0.28.0 under WSL and Android
2 participants