There is a possible race around generate_id and the use of the XC-MISC extension

[psychon]

I just saw https://gitlab.freedesktop.org/xorg/lib/libxcb/-/issues/51 and https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/118. Basically:
If you do let (a, b) = (conn.generate_id()?, conn.generate_id()?);, then a == b might hold. The first call returns the last available ID number, thus the XC-MISC extension is used to request a new batch of free IDs. Since the ID a is still unused from the POV of the server, the new range might include a again, thus a later call to generate_id() could return a again, causing an XID collision / BadIDChoice error. (But the way I describe it above with a == b is very, very unlikely...)

[eduardosm]

I can only think of two ways of avoiding this:

• With another function to generate multiple XIDs (which should make sure not to return repeated values).
• Calling generate_id just before the request that actually assigns the XID to a resource.

[psychon]

Option 1 requires people to carefully write code. Aka "someone will get it wrong".

Option 2 is not thread-safe (although I'd say this race is quite unlikely... I'd be curious to learn more about the C code that originally caused the issue with libX11...)

[psychon]

Dunno why, but I wrote a program to reproduce this (at least I found #783 in the process). The actual bug is hit in worker_thread which uses a sleep between generate_id and create_pixmap to have this hit the problem. I didn't manage to hit the bug without a sleep.

Output:

Allocating 2097150 pixmaps
..allocated 0 pixmaps
..allocated 100000 pixmaps
..allocated 200000 pixmaps
..allocated 300000 pixmaps
..allocated 400000 pixmaps
..allocated 500000 pixmaps
..allocated 600000 pixmaps
..allocated 700000 pixmaps
..allocated 800000 pixmaps
..allocated 900000 pixmaps
..allocated 1000000 pixmaps
..allocated 1100000 pixmaps
..allocated 1200000 pixmaps
..allocated 1300000 pixmaps
..allocated 1400000 pixmaps
..allocated 1500000 pixmaps
..allocated 1600000 pixmaps
..allocated 1700000 pixmaps
..allocated 1800000 pixmaps
..allocated 1900000 pixmaps
..allocated 2000000 pixmaps
Checking whether we exhausted XIDs correctly...
Trying to cause BadIdChoice with 4 threads...
thread 'main' panicked at 'Some(Error(X11Error { error_kind: IDChoice, error_code: 14, sequence: 41, bad_value: 4194302, minor_opcode: 0, major_opcode: 53, extension_name: None, request_name: Some("CreatePixmap") }))', x11rb/examples/badidchoice.rs:96:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

use x11rb::connection::Connection;
use x11rb::errors::ReplyOrIdError;
use x11rb::protocol::xproto;
use x11rb::wrapper::ConnectionExt;

const IDS_TO_KEEP_AVAILABLE: u32 = 1;
const NUM_THREADS: usize = 4;

fn create_pixmap(conn: &impl Connection, screen: usize) -> Result<u32, ReplyOrIdError> {
    let id = conn.generate_id()?;
    xproto::create_pixmap(conn, 1, id, conn.setup().roots[screen].root, 1, 1)?;
    Ok(id)
}

// Check that we really only have IDS_TO_KEEP_AVAILABLE XIDs left
fn check_id_exhaustion<C>(conn: &C, screen: usize) -> Result<(), ReplyOrIdError>
where
    C: Connection + x11rb::connection::RequestConnection,
{
    // Allocate all available IDs. This uses PixmapWrapper to free the pixmaps again automatically.
    let ids = (0..=IDS_TO_KEEP_AVAILABLE)
        .map(|_| {
            create_pixmap(conn, screen)
                .map(|pixmap| xproto::PixmapWrapper::for_pixmap(conn, pixmap))
        })
        .collect::<Result<Vec<_>, _>>()?;
    match conn.generate_id() {
        Err(x11rb::errors::ReplyOrIdError::IdsExhausted) => {
            // Yup, that's the error we want to see!
        }
        Ok(_) => panic!("Expected ID allocation to fail, but it succeeded!?"),
        Err(err) => panic!("ID allocation failed unexpectedly with {err:?}"),
    }
    // Now free the pixmaps again (silences a compiler warning)
    drop(ids);
    Ok(())
}

fn worker_thread<C>(conn: &C, screen: usize) -> Result<(), ReplyOrIdError>
where
    C: Connection + x11rb::connection::RequestConnection,
{
    let id = conn.generate_id();
    // Ignore IdsExhausted errors
    let id = match id {
        Err(x11rb::errors::ReplyOrIdError::IdsExhausted) => {
            println!("One thread sees IdsExhausted, this is not supposed to happen");
            return Ok(());
        }
        id => id?,
    };

    // How to make a race more likely? Insert a sleep!
    std::thread::sleep(std::time::Duration::from_micros(100));

    xproto::create_pixmap(conn, 1, id, conn.setup().roots[screen].root, 1, 1)?;
    Ok(())
}

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let (conn, screen) = x11rb::connect(None)?;
    let setup = conn.setup();
    assert_eq!(
        1,
        setup.resource_id_mask & 1,
        "I am too lazy to figure out what to do with shifted masks"
    );
    let num_ids_available = setup.resource_id_mask;
    let to_allocate = num_ids_available - IDS_TO_KEEP_AVAILABLE;
    println!("Allocating {to_allocate} pixmaps");
    for num in 0..to_allocate {
        create_pixmap(&conn, screen)?;
        if num % 100_000 == 0 {
            println!("..allocated {num} pixmaps");
        }
    }

    println!("Checking whether we exhausted XIDs correctly...");
    check_id_exhaustion(&conn, screen)?;

    println!("Trying to cause BadIdChoice with {NUM_THREADS} threads...");
    // Now start threads that allocate and free things
    std::thread::scope(|s| {
        let handles = (0..NUM_THREADS)
            .map(|_| s.spawn(|| worker_thread(&conn, screen)))
            .collect::<Vec<_>>();
        handles
            .into_iter()
            .map(|handle| handle.join().unwrap())
            .collect::<Result<Vec<_>, ReplyOrIdError>>()
    })?;

    conn.sync()?;

    let event = conn.poll_for_event()?;
    assert!(event.is_none(), "{event:?}");

    Ok(())
}