search

jordansissel / xdotool

fake keyboard/mouse input, window management, and more
Other
3.19k stars 317 forks source link

Add NULL check into xdo_get_desktop_for_window #434

Closed phil294 closed 1 year ago

phil294 commented 1 year ago

Hi!

I've been using libxdo.so for almost a year now as a static dependency of ahk_x11, using these bindings from @woodruffw in Crystal language. It works great!

However, there's one spot that repeatedly leads to crashes. Here's a sample stack trace:

XGetWindowProperty failed!Invalid memory access (signal 11) at address 0x0
[0x55f061b76c36] *Exception::CallStack::print_backtrace:Nil +118 in bin/ahk_x11
[0x55f061b4c2f6] ~procProc(Int32, Pointer(LibC::SiginfoT), Pointer(Void), Nil) +310 in bin/ahk_x11
[0x7ff6d76ceab0] ?? +140698152921776 in /usr/lib/libc.so.6
[0x7ff6d80d47a8] xdo_get_desktop_for_window +120 in /usr/lib/libxdo.so.3
[0x7ff6d80d71c3] ?? +140698163442115 in /usr/lib/libxdo.so.3
[0x7ff6d80d7805] ?? +140698163443717 in /usr/lib/libxdo.so.3
[0x7ff6d80d79db] xdo_search_windows +219 in /usr/lib/libxdo.so.3
[0x55f061ecd7fb] *XDo#search<XDo::Search>:Array(XDo::Window) +459 in bin/ahk_x11
[0x55f061fa64fd] *Cmd::ControlFlow::IfWinExist#run<Run::Thread, Array(String)>:Bool +1549 in bin/ahk_x11
[0x55f061edff02] *Run::Thread#do_next:(Int32 | Nil) +19714 in bin/ahk_x11
[0x55f061b55383] ~procProc(Nil) +35 in bin/ahk_x11
[0x55f061c518e0] *Fiber#run:(IO::FileDescriptor | Nil) +112 in bin/ahk_x11
[0x55f061b4be16] ~proc2Proc(Fiber, (IO::FileDescriptor | Nil)) +6 in bin/ahk_x11
[0x0] ???

I can't tell for sure why this is happening and what causes it, but I think it is related to windows disappearing while the search is running when a desktop search argument is specified. I can reliably reproduce the error, but the setup is pretty complicated and hard to track down.

While I have disabled Crystal's GC, it is still possible that this memory error is specific to the language. But still, adding this small NULL check fixes it for good. So I'd be glad if you could merge it, so I don't have to build my local fork anymore.

Thanks!

jordansissel commented 1 year ago

Thanks for this patch! Patches like this are wonderful examples of how a tiny code change can represent days, weeks, or months of research and experiments ;)

I'm curious and confused why this fixed anything and why XGetWindowProperty is storing NULL for the prop pointer while still setting nitems above zero? That said, I trust your report that this improves things and am happy to merge it.