"Garbage" appended to window titles about fvwm3 HOT 15 CLOSED

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

from fvwm3.

Thanks for checking.

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

I'll get around to fixing it soon.

from fvwm3.

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!

from fvwm3.

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.

from fvwm3.

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.

from fvwm3.

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.

from fvwm3.

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.

Demo code.zip

from fvwm3.

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.

from fvwm3.

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?

from fvwm3.

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.

from fvwm3.

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.

from fvwm3.

Hey,

Fine.

I'll just +1 and remain ignorant.

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

from fvwm3.

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" ?

from fvwm3.

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

from fvwm3.

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.

from fvwm3.