From c1f1b38b17afef8053d3eca86eb45368f5a373db Mon Sep 17 00:00:00 2001 From: MarcusGrass Date: Fri, 19 Apr 2024 06:24:41 +0200 Subject: [PATCH] Update with x11-clipboard retrospective --- pages/Nav.md | 8 +- pages/retrospectives/X11ToXcb.md | 173 +++++++++++++++++++++++++++++++ src/main.rs | 1 + 3 files changed, 180 insertions(+), 2 deletions(-) create mode 100644 pages/retrospectives/X11ToXcb.md diff --git a/pages/Nav.md b/pages/Nav.md index 94fa7c8..c263d9d 100644 --- a/pages/Nav.md +++ b/pages/Nav.md @@ -17,5 +17,9 @@ I made things easier for myself and made navigation happen through this md-page - [Threads - Tiny-std has threading support](/threads.html) - [Static pie - Tiny-std can compile as static-pie](/static-pie.html) - [Keyboard SMP - Multithreading in your keyboard](/kbd-smp.html) -- [Rust kbd - Keyboard firmware in rust](rust-kbd.html) -- [Test](/test.html) \ No newline at end of file +- [Rust kbd - Keyboard firmware in rust](/rust-kbd.html) +- [Test](/test.html) + +## Retrospectives + +- [RIIR, postmortem on changing out C-bindings for a pure Rust implementation](/x11-to-xcb.html) diff --git a/pages/retrospectives/X11ToXcb.md b/pages/retrospectives/X11ToXcb.md new file mode 100644 index 0000000..2268f20 --- /dev/null +++ b/pages/retrospectives/X11ToXcb.md @@ -0,0 +1,173 @@ +# Rewrite it in Rust, a cautionary tale + +RIIR (Rewrite It In Rust) is a pretty fun joke, at my current workplace my team writes +essentially everything in Rust, for good and bad. We like to have a bit of +fun with it, pushing the RIIR-agenda around the company. + +But, this short retrospective is about when porting something from C-bindings to Rust +just made life harder. + +## Security advisory on Rust XCB-bindings + +I've written a lot about [XCB](https://en.wikipedia.org/wiki/XCB) and +[X11](https://en.wikipedia.org/wiki/X_Window_System) in my project write-ups +about my [x11-wm](https://github.com/MarcusGrass/pgwm), I'm not going +to get into it here, but for these purposes `XCB` can be summarized as a +library to handle displaying things on a desktop. + +One day when building a project, a security advisory comes up on [Rust's XCB bindings](https://github.com/rust-x-bindings/rust-xcb). + +### Bindings + +Generally if you want to use an existing big library, you can take the approach of reinventing the wheel, +or creating bindings to a C-library that already exists. For example, +Rust has a [zstd crate](https://crates.io/crates/zstd) which contains bindings +to [libzstd](https://github.com/facebook/zstd). If you want to use that, +you need to have `libzstd` available to the binary. Sometimes, it's built as +part of a build-script and statically compiled into the binary, then you don't +have to worry about it at all ([Rocksdb does this I think](https://docs.rs/rocksdb/latest/rocksdb/)). +There's also a pure Rust implementation of [zstd decompression](https://github.com/KillingSpark/zstd-rs), +which is the other approach, same algorithm, different implementation. + +### Why not? + +There are some good reasons to RIIR, all the good things about using Rust can go here. +But, there are some very good reasons not to, apart from the effort. +The one this retrospective is about is maturity, and the robustness that can come from it. + +### Porting x11-clipboard from C-bindings to Rust implementation + +The security advisory comes up, transitively through [x11-clipboard](https://github.com/quininer/x11-clipboard), +but the advisory is on the `XCB`-bindings. +As I mentioned, my previous work on my WM had made me familiar with a Rust +library that replaces the bindings: [x11rb](https://github.com/psychon/x11rb). + +To be clear, `x11rb` is a great library, and the story is not about how it contained some unexpected bug, +it didn't, it was the act of replacement that became the issue. + +[I made a PR on June16, 2022](https://github.com/quininer/x11-clipboard/pull/29) to replace usage of the bindings, to +`x11rb` in `x11-clipboard. The PR is fairly large, but very procedural. The rust-api +is essentially the same as the C-one, it was mostly a matter of changing the types. + +## Creeping issues + +`x11-clipboard` is a library that handles copying and pasting withing x11-sessions. It's used for a lot of Rust's +gui-applications, so people are likely to run into mistakes if you make them, and there were mistakes. + +### Bug report through alacritty + +9 months later, [alacritty gets a bug report](https://github.com/alacritty/alacritty/issues/6760), where +when things are pasted FROM alacritty into other applications, they hang. + +The bug report is floated into [x11-clipboards issue tracker](https://github.com/quininer/x11-clipboard/issues/33) after +a bisection shows that the problem comes from the version update caused by my change. + +Debugging it was medium-difficult, it was easy to reproduce, but difficult to understand, but in the end it was +resolved by a [+1 -1 change](https://github.com/quininer/x11-clipboard/pull/34), + +From this: +```rust + time: event.time, + requestor: event.requestor, + selection: event.selection, + target, + property: event.property + } +); +``` + +To this: +```rust + time: event.time, + requestor: event.requestor, + selection: event.selection, + target: event.target, + property: event.property + } +); +``` + +The error was interesting, it caused some clients (a client in this context is an application like +[Brave browser](https://brave.com/)) to hang waiting for a notification that the application never sent. + +A funny note about `X11` is that the protocol has been around for so long, and seen so much misuse, that a lot of +clients are built to handle this kind of mistake, so the error doesn't show up on for example +[Firefox](https://www.mozilla.org/en-US/firefox/new/). + +### Bug report through pot-app + +[On Jan 17, 2024 a bug report comes in from pot-app/Selection](https://github.com/pot-app/Selection/issues/3). + +[Pot App is:](https://github.com/pot-app) + +> 🌈一个跨平台的划词翻译和OCR软件 | A cross-platform software for text translation and recognition. + +To be fair, I think this was a pre-existing bug, but I was kind of on the hook at this point, and it was interesting. + +The clipboard library spawns a thread that listens for events, this threads holds a claim to the connection to the +x-server, blocking waiting for a reply. Even if the handle that's given to the user is dropped that thread stays alive, keeping +the connection alive. This means that if you're recreating the structure in a loop for example, you start leaking +connections until the connection-pool is drained, which means that no new clients can connect. Or in other words, +no more applications can start because you clogged up the server. + +A problem here is that the thread needs to know from the structure that spawned it, that it's done and should quit. +There are not many nice way of signalling threads like that they are blocked waiting for something. + +The thread waits like this: + +```rust +while let Ok(event) = context.connection.wait_for_event() { +``` + +The API doesn't have other facilities for waiting other than polling in a loop, and ideally one doesn't want to +run the thread at 100% CPU just waiting. + +However, you can get the underlying file descriptor for the connection like this: + +```rust +let stream_fd = context.connection.stream().as_fd(); +``` + +And if you have an FD, you can use Linux's APIs to check for readiness, instead of what's exposed through the +`x11rb` API. This is only running on Linux anyway, so why not? (This is foreshadowing). + +In the end [I make a PR](https://github.com/quininer/x11-clipboard/pull/46) that uses [libc](https://github.com/rust-lang/libc), +the [Linux Poll API](https://man7.org/linux/man-pages/man2/poll.2.html), and an, +[eventfd](https://man7.org/linux/man-pages/man2/eventfd.2.html). +If the struct is dropped, it'll write an event on the `eventfd`. On the other side, the thread polls for +either a new message on the stream, or an event on the `eventfd`, if an event arrives on the stream, it'll handle that +like before, if it arrives through the `eventfd` it just quits. That solved the issue. + +### Bug report through the regular issue tracker + +[On Feb 28, 2024 a bug report is posted on x11-clipboard](https://github.com/quininer/x11-clipboard/issues/48). + +Now, I figured `X11` was only used on Linux, Mac and Windows have their own display systems. But, I forgot about +the BSDs, those operation systems can run `X11`, and I should have thought about that before picking the +Linux specific `eventfd`. + +#### POSIX + +[POSIX](https://en.wikipedia.org/wiki/POSIX) is an OS-compatibility standard, if you use POSIX-compliant OS-apis, +can generally get away with using APIs that interface with the OS for Linux and they'll still work for the BSDs, +some examples: `poll`, [read](https://man7.org/linux/man-pages/man2/read.2.html), [write](https://man7.org/linux/man-pages/man2/write.2.html), +[pipe](https://man7.org/linux/man-pages/man2/pipe.2.html). `eventfd` is a counter-example. + +What my bugfix was trying to achieve was a drop of the struct exposed through the `x11-clipboard` API causing something +pollable to happen in the running thread. I thought `eventfd` was a good fit, but something POSIX-compliant would be +to create a `pipe`, two fds, a read-end and a write-end, put the write-end in the user struct, the read-end in the +thread, and poll for a `POLLHUP` (hangup), that gets sent to one end when the other end's `FD` is closed. + +Now I could use the existing RAII-closing of the write-end on the user struct, and just listen to a hangup on the running +thread, and it works on the BSDs! + +## Conclusion + +For now that's been it, I'll update this if more stuff comes in. I think that lessons learned here are that there's a +maintenance cost to any change. While RIIR might be fun, it's good to think twice about how reasonable it is. + +Of course, there may be lurking bugs in the C-implementation that isn't seen because of selection bias, but I don't have +any basis for that. + +Last of all, I'm sorry for the hassle [quininer](https://github.com/quininer), I know you don't want to maintain this +project anymore, and I made your life a bit more difficult. diff --git a/src/main.rs b/src/main.rs index 858269d..66a1702 100644 --- a/src/main.rs +++ b/src/main.rs @@ -18,6 +18,7 @@ const LOCATIONS: &[LocationInfo] = &[ LocationInfo::new("/static-pie.html", "StaticPie", "static-pie"), LocationInfo::new("/kbd-smp.html", "KbdSmp", "kbd-smp"), LocationInfo::new("/rust-kbd.html", "RustKbd", "rust-kbd"), + LocationInfo::new("/x11-to-xcb.html", "X11ToXcb", "x11-to-xcb"), LocationInfo::new("/test.html", "Test", "test"), ];