| Summary: | xwininfo -name finds wrong window | ||||||
|---|---|---|---|---|---|---|---|
| Product: | xorg | Reporter: | Nadav Har'El <nyh> | ||||
| Component: | App/xwininfo | Assignee: | Alan Coopersmith <alan.coopersmith> | ||||
| Status: | RESOLVED FIXED | QA Contact: | Xorg Project Team <xorg-team> | ||||
| Severity: | normal | ||||||
| Priority: | medium | CC: | mcepl | ||||
| Version: | unspecified | ||||||
| Hardware: | Other | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| i915 platform: | i915 features: | ||||||
| Attachments: |
|
||||||
|
Description
Nadav Har'El
2011-05-29 05:01:19 UTC
I just compiled the latest xwininfo myself (http://www.x.org/releases/X11R7.6/src/app/xwininfo-1.1.1.tar.gz) and the bug remains there. It appears that the code I previously looked at wasn't the latest code - the latest code appears to be using XCB instead of XLIB - I didn't know that. It is sad for me to see how this simple application where speed is of no importance became so convoluted by using XCB :( It has been so long since I last read X sources, that I didn't even know this was happening. Anyway, the bug in the new code I found now is pretty obvious: the check prop_name_len = xcb_get_property_value_length (prop); strncmp (prop_name, name, prop_name_len) is wrong! If prop_name is "", it checks zero characters, and obviously finds a match ;-) It should have used name's length, not prop_name's length! There are two other instances of this wrong check in the code. Using strlen(name) in all calls of strncmp in dsimple.c fixed this bug. Please apply this fix. Nadav. On second thought, using strlen(name), while better, is also not accurate - it can cause "xwininfo -name hell" to find a window named "hello" :-) We need a new comparison function which 1. First compares the lengths, 2. And only if the lengths are equal, call strncmp. Again, this is a very simple fix, and quite important: I was using "xwininfo -name ..." to find windows with a certain name and kill them, and then, after upgrading my system, the script which used to work well started to kill my Firefox windows :( (In reply to comment #1) > Please apply this fix. Could you create a patch, please? Thank you Created attachment 47314 [details] [review] Fixes the bug described above. Here is my proposed fix for the bug: In addition to strncmp, we also need to check that the lengths are identical - otherwise any string matches with an empty window name :-( (In reply to comment #4) > Created an attachment (id=47314) [details] > Fixes the bug described above. > > Here is my proposed fix for the bug: In addition to strncmp, we also need to > check that the lengths are identical - otherwise any string matches with an > empty window name :-( I am sorry, I was reprimanded that xorg doesn't use patches-as-attachments to bugs, but you have to follow http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches. I am sorry for the confusion again. The well known saying is that you can lead a horse to water, but you cannot make it drink. In this case, I found a rather serious regression in the "xwininfo" tool, explained where the bug is, and even wrote code to fix it. I think that for anyone that cares about this code, this is more than enough, even if it doesn't follow the accurate patch submission instructions, whatever these are... After all, this is a bug report, not a new feature request. When I tried to understand why xwininfo, a decades-old tool, is suddenly written in XCB and not Xlib, I found that (see http://lists.freedesktop.org/archives/xcb/2010-June/006111.html), the author "mostly did this as a way to learn more xcb". I hope he continues to care about the code, and improve his learning :-) (In reply to comment #6) > The well known saying is that you can lead a horse to water, but you cannot > make it drink. Sure, of course, I (or more likely Alan) can send that email to the list myself, I just didn't want to steal fame and ohloh.net points from you. Adding Alan to CC of this bug. Thanks for the cc, but I get xorg-team mail, so already got all of it. Patches to the mailing list are preferred but I can easily take the patch from bugzilla & apply it. Thanks for doing the work to diagnose this. While I did the port to xcb so I could learn it better, that's not the only reason for having it ported - it does improve performance over low latency and helps serve as a xcb test case and example. Patch proposing a similar fix to the one in attachment 47314 [details] [review] submitted to xorg-devel for review: http://patchwork.freedesktop.org/patch/5818/ Fix pushed to git master: http://cgit.freedesktop.org/xorg/app/xwininfo/commit/?id=456b0fafa73f1227adf07ee924b316cbf9fe83cb |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.