segfault in xcb_util::icccm::GetWmClassReply::instance

[bennyandresen]

Hey,

as per IRC:

Program received signal SIGSEGV, Segmentation fault.
_______________________________________________________________________________
     eax:00000000 ebx:00000000  ecx:00000000  edx:00000001     eflags:00010293
     esi:9CA02140 edi:00000000  esp:C737F948  ebp:C737F960     eip:Error while running hook_stop:
Value can't be converted to integer.
0x00007fb49d825446 in strlen () from /usr/lib/libc.so.6
(rr) bt
#0  0x00007fb49d825446 in strlen () from /usr/lib/libc.so.6
#1  0x000055ae5727fa2e in std::ffi::c_str::CStr::from_ptr () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/ffi/c_str.rs:505
#2  0x000055ae572768f0 in xcb_util::icccm::GetWmClassReply::instance (self=0x7fffc737fa40) at /home/benny/.cargo/git/checkouts/rust-xcb-util-88b25b1b10c435bb/master/src/icccm.rs:193
#3  0x000055ae57229dc8 in i3fu::get_instance (connection=0x7fffc73805d0, wid=0x3000001) at /home/benny/Projects/i3/i3fu/src/main.rs:78
#4  0x000055ae5722971a in i3fu::main () at /home/benny/Projects/i3/i3fu/src/main.rs:67
#5  0x000055ae572905db in panic_unwind::__rust_maybe_catch_panic () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libpanic_unwind/lib.rs:97
#6  0x000055ae5728728b in std::panicking::try<(),fn()> () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:332
#7  std::panic::catch_unwind<fn(),()> () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panic.rs:311
#8  std::rt::lang_start () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/rt.rs:57
#9  0x000055ae5722a174 in main ()
(rr) x 0x7fffc737fa40
0x7fffc737fa40: 0x00000000

I don't have a short reproducible case yet, but here is the rust code that causes it:

fn get_instance(connection: &xcb::Connection, wid: xcb::Window) -> String {
    let reply = xcb_util::icccm::get_wm_class(connection, wid).get_reply();

    match reply {
        Ok(r) => r.instance().to_string(),
        Err(_) => "".to_string()
    }
}

I'll try to see if I can make it reproducible.

[bennyandresen]

Output from bt full is more illuminating

I can fix it in my code, but maybe it should require to be wrapped in unsafe {} so one knows this sort of thing can happen?

#2  0x000055ae572768f0 in xcb_util::icccm::GetWmClassReply::instance (self=0x7fffc737fa40) at /home/benny/.cargo/git/checkouts/rust-xcb-util-88b25b1b10c435bb/master/src/icccm.rs:193
        self = 0x7fffc737fa40
#3  0x000055ae57229dc8 in i3fu::get_instance (connection=0x7fffc73805d0, wid=0x3000001) at /home/benny/Projects/i3/i3fu/src/main.rs:78
        r = xcb_util::icccm::GetWmClassReply (
          xcb_util::ffi::icccm::xcb_icccm_get_wm_class_reply_t {
            instance_name: 0x0,
            class_name: 0x0,
            _reply: 0x0
          }
        )
        reply = core::result::Result::Ok(xcb_util::icccm::GetWmClassReply (
            xcb_util::ffi::icccm::xcb_icccm_get_wm_class_reply_t {
              instance_name: 0x0,
              class_name: 0x0,
              _reply: 0x0
            }
          ))
        connection = 0x7fffc73805d0
        wid = 0x3000001

[meh]

That's really weird, especially the fact _reply is NULL.

[psychon]

Random guess: You are ignoring the return value of xcb_icccm_get_wm_class_reply. However, this function can fail for any number of reasons, e.g. the window was already destroyed: https://sources.debian.org/src/xcb-util-wm/0.4.1-1.1/icccm/icccm.c/#L338-L339

  if(!reply || reply->type != XCB_ATOM_STRING || reply->format != 8)
    return 0;

Worse, it can fail because the connection is in an error state (e.g. maximum request length was exceeded). In this state, there is no reply and no X11 error either. The only indication is the return value of the called function, which you ignore: https://github.com/meh/rust-xcb-util/blob/a87753c33ec875832f860596536ea97f22fd6a21/src/util.rs#L50-L54 In this case, mem::zeroed() is the only thing that writes to reply, which matches the symptoms above.

Some examples on how to get an XCB connection into an error state: https://github.com/rtbo/rust-xcb/issues/64

[meh]

@psychon thanks for that, I started checking the return value from the various *_reply functions, do you think I missed anything?

[psychon]

No idea if you handled all cases, but from a quick look: Is Err(xcb::GenericError { ptr: null() }) a good idea? If I understand the latest commit correctly, your patch creates GenericError instances containing null pointer. That can easily result in NULL pointer dereferences:

https://github.com/rtbo/rust-xcb/blob/9dd7b029edcc95fb8acfe79c8ca1bfa52b1b9ae4/src/base.rs#L137-L150

[meh]

Right, at least it's not as bad when the GenericError gets dropped because it just calls libc::free.

I think I need something to be done in the xcb crate in this case because I don't want to build a dummy xcb_generic_error_t to feed to GenericError and this seems to be an error case that hadn't been considered.

For now this is still better than exploding completely I guess.

[psychon]

Possibly related: https://github.com/rtbo/rust-xcb/pull/66

You could use ReplyError in your code as well when/if this PR is merged in its current form.

[meh]

Yeah, that looks like the right way to go.