Skip to content

Conversation

En-En-Code
Copy link
Contributor

Identifies a flaw in xcb::Connection::connect_to_fd and xcb::Connection::connect_to_fd_with_extensions which allows file descriptors to be used after close and/or double closed. An issue was raised back in 2022 to mark these functions as unsafe with no additional context. I discovered this unsoundness causing problems working on a different project and reported my findings to xcb.
It is unclear as to whether the project is maintained, though previously there was an issue raised as to its maintained status (see #653).
I did not put any categories or keywords. I just did not know what to put for them.

@djc
Copy link
Contributor

djc commented Aug 13, 2025

I've emailed @rtbo personally (they seem somewhat active on GitHub at least), let's see if they respond in the next two weeks. If not, we should go ahead and merge the advisory (please help remind me).

@rtbo
Copy link

rtbo commented Aug 14, 2025

Hi,
I've honestly little time to allocate to the maintenance of rust-xcb, but I don't want it to die unmaintained either!
I can definitely share a write access to the repo.

@rtbo
Copy link

rtbo commented Aug 16, 2025

Should be closed by rust-x-bindings/rust-xcb#283

@djc
Copy link
Contributor

djc commented Aug 16, 2025

One thing to mention is that this isn't a memory safety violation but rather an I/O safety violation. I think that may warrant tagging it with a dedicated category? It also wasn't clear to me from reading the discussions linked from that RFC whether I/O safety violations can cause undefined behavior, and thus whether this qualifies as "unsound" at all.

That's a good point, this probably doesn't qualify as unsound.

@En-En-Code
Copy link
Contributor Author

Sorry for not having a quicker response. I guess I can give my opinions (to be taken with a grain of salt).

As long as someone can respond within a reasonable amount of time to major xcb issues, I have no problem calling it maintained. It's clear to me that has been done here.

Maybe I am missing something, but the neither the Reference nor the Nomicon nor the FLS expressly state that calling foreign code in a way which causes undefined behavior in the foreign code (for whatever its definition of UB may be) to be undefined behavior in Rust. UB should cross FFI boundaries, because the opposite is absurd. That was my defense for calling this unsound: the POSIX standard says closing or using a file descriptor after close is UB, safe functions provided by xcb enable that to occur.

I/O safety is in this weird place. Its main PR landed 3 years ago, but no one ever defined what constitutes UB when using a RawFd. The functions in the standard library which takes a RawFd and return some other file-like type, BorrowedFd::borrow_raw and FromRawFd::from_raw_fd, are marked unsafe to enforce I/O safety. But with I/O safety not being "real" (in the sense that it also is not defined in any of the aforementioned places) and RawFds usage being almost completely exclusive to FFI (where misuse should be an unsoundness anyways), I/O safety never got a category. It doesn't even have a keyword somehow.

I believe the nix crate to be in the wrong, and the lack of a rigorous I/O safety definition contributing to it sticking around. The equivalent function in rustix, rustix::io::close, is marked unsafe. More broadly, I believe most "safe" functions which allow the user to provide a RawFd and do syscalls with it are unlikely to be sound. As example,

fn main() {
    let file_fd = File::create_new("hello_world.txt").unwrap();
    let raw_fd = file_fd.as_raw_fd();
    drop(file_fd);
    // This is sound, a RawFd is just a number
    println!("raw_fd: {raw_fd:?}");
    // I believe this should be undefined if it does something undefined in C.
    unsafe { libc::some_syscall(raw_fd) };
}

Finally, I've made changes to the advisory now that a fixed version is out. I have still left it with no keyword and informational = unsound since there seems to be no consensus on a more apt label.

@djc djc merged commit bf013bc into rustsec:main Aug 22, 2025
1 check passed
@djc
Copy link
Contributor

djc commented Aug 22, 2025

Thanks!

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.

3 participants