"Garbage" appended to window titles

[Zirias]

Upfront Information

• Fvwm3 version (run: fvwm3 --version)

  fvwm3 1.0.7 (released)
  with support for:  ReadLine, XPM, PNG, SVG, Shape, XShm, SM, Bidi text, XRandR, XRender, XCursor, XFT, NLS

• Linux distribution or BSD name/version FreeBSD 13.2-RELEASE

• Platform (run: uname -sp) FreeBSD amd64

Actual Behaviour

Some window titles (seemingly random) get garbage appended. Sometimes, this is obviously text from some other window title (or maybe any other internally used string?), so just maybe it might be an issue with C string termination? I've seen things like for example: Here's another example where I have no idea where the extra text came from: and sometimes, it's indeed just utter "garbage characters".

Steps to Reproduce

Unfortunately, it's hard to tell. Use fvwm3 for a while actively with many open windows...

Does Fvwm3 crash?

So far, it did not.

Extra Information

I have never seen this issue in fvwm3 1.0.6

[ThomasAdam]

Yep, it's due to b7ae07c34bb9172b8aba964023c2febc178f7f97 -- if you revert that, does the problem go away?

[Zirias]

Yep, it's due to b7ae07c -- if you revert that, does the problem go away?

Thanks, building a package with that commit reverted right now. I guess I'll have to use my desktop for 1 or 2 days until I can be sure, will report back!

[Zirias]

Many hours of active usage and I didn't spot a single case, that's probably enough to assume reverting this commit indeed solves the issue.

Does this break anything else? If not, I would add it as a quick fix to the FreeBSD port until a fixed version is released.

[ThomasAdam]

Thanks for checking.

No harm in reverting this for now, if you want to.

I'll get around to fixing it soon.

[Ndolam]

I have had a similar problem in 1.0.7, and I poked around and discovered that if in atom_get() in ewmh.c I (a) make the fxmalloc() one longer: data = fxmalloc(num_ret asize + 1); (b) and add one to the memcpy() length memcpy(data, retval, num_ret asize + 1); then the problem goes away.

I don't know if this is the same issue as above, but I thought I'd add this comment here since it is related.

In my case, I was seeing this sporadically, and then I by chance happened to have a window whose title was 64 bytes long (not counting the \0 at the end) and found that the above changes made the problem go away.

I know nothing about X internals, and so I don't know whether num_retasize is supposed to include the \0 when the data is a string, but my debug statements show that the space for the \0 is not included in num_retasize. (In fact, maybe I shouldn't be memcpy()ing the extra byte, maybe I should explicitly put '\0' in data[num_ret * asize]. I will leave that to someone who knows about X internals.

[Ndolam]

By the way, to reproduce, I have found that having a window title of 32 or 64 chars reliably reproduces the bug. I (wildly) speculate that malloc() on my system allocates memory in multiples of 32-byte chunks, and when the title is a multiple of 32 bytes long, the title is not nul-terminated and you get whatever garbage follows the allocated chunk. (And for other lengths, I wildly guess that after you have been using your system for a while, fvwm3's heap gets lots of non-nul bytes scattered through it, and you end up seeing some of those in these other cases.

[polarbub]

I have had the same issue. It seems to be the first title set by the window gets extra random chars. I noticed it on the kate text editor, Intellij based ides, a random Java GUI program I was working on. Does not happen in 1.0.6a. Attached is the code that I noticed the issue with. Pressing ESC changes the title to show how changing it fixed the issue.

https://github.com/fvwmorg/fvwm3/assets/71790726/c975ae84-a8da-4ccc-a172-dbc4f4a6a278

Demo code.zip

[ThomasAdam]

Hey all,

Having looked into this a little more, I think the problem is in how we're calculating the atom size. Rather than shifting to the right 3, which would give us a size of 1 for formats which fit within a certain length, just use this length as-is.

Have a look at the ta/gh-873 branch and let me know if this fixes the problem for you.

[Ndolam]

Does this not end up allocating way more space than necessary for window titles? Not that that is the end of the world. Is adding one more byte for the \0 not sufficient?

[ThomasAdam]

Yes it does. I still need to adjust the offset.

The problem with just adding one to an arbitrary value is the calculation we currently use assumes 32-bit boundary chunks which needs aligning better.

[Ndolam]

Ummm... Is it the case that atom_size() is intended to return the number of bytes corresponding to the format, or is the meaning of that function something else? If the number of bytes, could it not just return format % 8
(or format >> 3) in all cases? My reading of the XGetWindowProperty() documentation tells me that format_ret can only be 32, 16 or 8, so I am not seeing why atom_size() has two cases. In fact, if some architecture has sizeof(long) to be something other than 4, and atom_size(32) is called, I think the wrong result would be returned. If, that is, atom_size() is supposed to return the number of bytes.

I'm a bit confused by your use of "arbitrary value". Surely the number of bytes to allocate is not arbitrary, it is the number of bytes that XGetWindowProperty returns for the given atom, is it not?

Further, the following statement doesn't seem to make sense: if (format_ret == 32 && asize * 8 != format_ret) Suppose format_ret is 32. Then asize will be 4 (on many architectures, anyway). Then asize*8 will be 32, and the second conjunct will be false. So it seems to me that the "then" clause will never be used, but the "false" one will.

Finally (?), about the missing NUL at the end of the string: https://www.x.org/releases/current/doc/man/man3/XGetWindowProperty.3.xhtml notes the following:

XGetWindowProperty always allocates one extra byte in prop_return (even if the property is zero length) and sets it to zero so that simple properties consisting of characters do not have to be copied into yet another string before use.

So I still am thinking that all that is needed is to add one byte to the string and memcpy() one extra byte.

But if I am wrong I am happy to be straightened out.

[ThomasAdam]

Hey,

Fine.

I'll just +1 and remain ignorant.

Again, check out the ta/gh-873 branch.

[Ndolam]

I went looking for ta/gh-873, but either it is gone or I don't know where to look.

I think if you are keeping track of some To-Dos, you might want to add "review code in ewmh_ChangeProperty() to see if it is ever possible to execute the code inside the "datacopy" if block, and, if it is possible to execute that, does an extra byte need to be allocated for datacopy and set to 0" ?

[polarbub]

It got merged into the main branch. See commit 4d5a697.

[Ndolam]

OK, it wasn't clear to me that that commit was the entirety of the interesting stuff in ta/gh-873. Thanks.

But the other comment (about ewmh_ChangeProperty()) I believe remains.