Re: Leak in XKeysymToString
Keith Packard writes: > Maybe something like this? That would work, yes. Thanks.
Re: Leak in XKeysymToString
On 8/21/22 18:20, Po Lu wrote: Alan Coopersmith writes: Okay, but we also document that Xlib is thread safe if XInitThreads() has been called, so both the patch suggested here to keep a static pointer to a malloc'ed buffer and my suggestion of a global static buffer fail since calls in different threads would have a race condition over whose answer got returned. We'd at least need a thread-specific buffer, which it doesn't look like we've done in Xlib so far. In modern C that is as simple as using _Thread_local, I think. I don't know if all our supported platforms support C11 yet - most will, but even getting to full C99 support took years with holdouts from certain NetBSD & OpenBSD platforms, and Microsoft's compilers. (This is a code base that predates C89 by 5 years and was originally written to K C, so C99 is pretty modern for us.) -- -Alan Coopersmith- alan.coopersm...@oracle.com Oracle Solaris Engineering - https://blogs.oracle.com/solaris
Re: Leak in XKeysymToString
Alan Coopersmith writes: > Thanks - while gitlab is our preferred method, when that's not possible, > we prefer using the xorg-devel mailing list (cc'ed) instead of trying to > guess which individual developer to contact. Thanks for explaining. I thought xorg-devel was shut down and replaced by Discourse, but I'm probably confusing it with GNOME lists. > This bug has been previously reported, but no one has developed a good > fix yet - I don't know if many XKeysymToString callers keep references to > the returned pointers and would be broken if those pointers suddenly had a > different string or were invalid due to a realloc() call. FWIW, libxkbfile (which triggers this bug) does not. I haven't seen code do that myself either. > If they do, then perhaps a safer fix would be to keep a list of the > returned strings and reuse the pointer when the same keysym is seen again > instead of continuing to allocate new memory each time. Probably in an assoc table or similar structure, right? Using a plain list seems horribly inefficient once XKeysymToString is called for enough keysyms. > The safest fix would be to just have makekeys generate these values > when building the keysym tables, but that would increase the memory > usage for those tables. That could work too, yes. > If not, and we decided overwriting the string each time was acceptable, > I'd just go with "static char s[10];" instead of malloc/realloc at all. Sure.
Re: Leak in XKeysymToString
On 8/20/22 11:47, Thomas Dickey wrote: On Sat, Aug 20, 2022 at 09:51:42AM -0700, Alan Coopersmith wrote: Thanks - while gitlab is our preferred method, when that's not possible, we prefer using the xorg-devel mailing list (cc'ed) instead of trying to guess which individual developer to contact. This bug has been previously reported, but no one has developed a good fix yet - I don't know if many XKeysymToString callers keep references to the returned pointers and would be broken if those pointers suddenly had a different string or were invalid due to a realloc() call. The manpage hints that callers should make a copy of the string, since that "static area" implies that the library overwrites the data for each call: The returned string is in a static area and must not be modified. The returned string is in the Host Portable Character Encoding. If the specified KeySym is not defined, XKeysymToString returns a NULL. Okay, but we also document that Xlib is thread safe if XInitThreads() has been called, so both the patch suggested here to keep a static pointer to a malloc'ed buffer and my suggestion of a global static buffer fail since calls in different threads would have a race condition over whose answer got returned. We'd at least need a thread-specific buffer, which it doesn't look like we've done in Xlib so far. -- -Alan Coopersmith- alan.coopersm...@oracle.com Oracle Solaris Engineering - https://blogs.oracle.com/solaris
Re: Leak in XKeysymToString
On 8/20/22 18:20, Po Lu wrote: Alan Coopersmith writes: Thanks - while gitlab is our preferred method, when that's not possible, we prefer using the xorg-devel mailing list (cc'ed) instead of trying to guess which individual developer to contact. Thanks for explaining. I thought xorg-devel was shut down and replaced by Discourse, but I'm probably confusing it with GNOME lists. Right - GNOME did that, but X.Org still uses mailing lists. If they do, then perhaps a safer fix would be to keep a list of the returned strings and reuse the pointer when the same keysym is seen again instead of continuing to allocate new memory each time. Probably in an assoc table or similar structure, right? Using a plain list seems horribly inefficient once XKeysymToString is called for enough keysyms. Right. -- -Alan Coopersmith- alan.coopersm...@oracle.com Oracle Solaris Engineering - https://blogs.oracle.com/solaris
Re: Leak in XKeysymToString
On Sat, Aug 20, 2022 at 09:51:42AM -0700, Alan Coopersmith wrote: > Thanks - while gitlab is our preferred method, when that's not possible, > we prefer using the xorg-devel mailing list (cc'ed) instead of trying to > guess which individual developer to contact. > > This bug has been previously reported, but no one has developed a good > fix yet - I don't know if many XKeysymToString callers keep references to > the returned pointers and would be broken if those pointers suddenly had a > different string or were invalid due to a realloc() call. The manpage hints that callers should make a copy of the string, since that "static area" implies that the library overwrites the data for each call: The returned string is in a static area and must not be modified. The returned string is in the Host Portable Character Encoding. If the specified KeySym is not defined, XKeysymToString returns a NULL. -- Thomas E. Dickey https://invisible-island.net ftp://ftp.invisible-island.net signature.asc Description: PGP signature
Re: Leak in XKeysymToString
Thanks - while gitlab is our preferred method, when that's not possible,
we prefer using the xorg-devel mailing list (cc'ed) instead of trying to
guess which individual developer to contact.
This bug has been previously reported, but no one has developed a good
fix yet - I don't know if many XKeysymToString callers keep references to
the returned pointers and would be broken if those pointers suddenly had a
different string or were invalid due to a realloc() call.
If they do, then perhaps a safer fix would be to keep a list of the
returned strings and reuse the pointer when the same keysym is seen again
instead of continuing to allocate new memory each time. The safest fix
would be to just have makekeys generate these values when building the
keysym tables, but that would increase the memory usage for those tables.
If not, and we decided overwriting the string each time was acceptable,
I'd just go with "static char s[10];" instead of malloc/realloc at all.
-alan-
On 8/20/22 01:56, Po Lu wrote:
Here's an untested fix for what I think is a leak in XKeysymToString.
I'm sorry if this is not the right way to reach an Xlib developer, but I
don't have an account on gitlab.freedesktop.org, and creating one is
difficult for me.
Thanks.
diff --git a/src/KeysymStr.c b/src/KeysymStr.c
index 8fe1bd64..a4f1c10a 100644
--- a/src/KeysymStr.c
+++ b/src/KeysymStr.c
@@ -120,13 +120,16 @@ char *XKeysymToString(KeySym ks)
}
if (ks >= 0x01000100 && ks <= 0x0110) {
KeySym val = ks & 0xff;
-char *s;
+static char *s;
int i;
if (val & 0xff)
i = 10;
else
i = 6;
-s = Xmalloc(i);
+ if (!s)
+ s = Xmalloc(i);
+ else
+ s = Xrealloc(s, i);
if (s == NULL)
return s;
i--;
--
-Alan Coopersmith- alan.coopersm...@oracle.com
Oracle Solaris Engineering - https://blogs.oracle.com/solaris
