Bug 37710 - xwininfo -name finds wrong window
Summary: xwininfo -name finds wrong window
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: App/xwininfo (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Alan Coopersmith
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-29 05:01 UTC by Nadav Har'El
Modified: 2018-06-17 20:01 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Fixes the bug described above. (1.56 KB, patch)
2011-05-30 06:56 UTC, Nadav Har'El
no flags Details | Splinter Review

Description Nadav Har'El 2011-05-29 05:01:19 UTC
if I try 

$ xwininfo -name ZZZ

where ZZZ is some name for which no window exists, I get a wrong window, one which happens to belong to a Firefox 4 I am running. Clearly, this window's name *isn't* ZZZ - rather it has no name:

$ xwininfo -name ZZZ

xwininfo: Window id: 0x2a0006d (has no name)

  Absolute upper-left X:  0
  Absolute upper-left Y:  0
  Relative upper-left X:  0
  Relative upper-left Y:  0
  Width: 200
  Height: 200
...

I get the same window regardless of which non-existant name I use. I never get the error message I should have gotten in this case.

Looking at the code, in dsimple.c's Window_With_Name(), I cannot find an obvious reason for this bug. However, I think I found another unrelated oversight there: You use XFetchName, but forget to XFree window_name after you used it.

By the way, I'm using Fedora 15 and xorg-x11-utils-7.5-2.fc15.x86_64
Comment 1 Nadav Har'El 2011-05-29 06:08:49 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.
Comment 2 Nadav Har'El 2011-05-29 08:19:24 UTC
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 :(
Comment 3 Matej Cepl 2011-05-30 05:09:05 UTC
(In reply to comment #1)
> Please apply this fix.

Could you create a patch, please?

Thank you
Comment 4 Nadav Har'El 2011-05-30 06:56:26 UTC
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 :-(
Comment 5 Matej Cepl 2011-05-30 14:36:32 UTC
(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.
Comment 6 Nadav Har'El 2011-05-30 23:44:55 UTC
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 :-)
Comment 7 Matej Cepl 2011-05-31 00:19:04 UTC
(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.
Comment 8 Alan Coopersmith 2011-05-31 08:42:29 UTC
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.
Comment 9 Alan Coopersmith 2011-06-07 23:19:02 UTC
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/


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.