x11: Prevent memory corruption in XrmSetDatabase

[cdown]

Despite what the XrmSetDatabase docs say, it may try to free the database, resulting in memory corruption. Prevent that by making sure it has no db to act on. If it's past the first run, we will have done XrmDestroyDatabase above anyway. This causes a segfault, reported in issue #1256.

From the XrmSetDatabase man page:

The database previously associated with the display (if any) is not destroyed.

But this is patently false, from the source in Xlib's src/Xrm.c:

void XrmSetDatabase(
    Display *display,
    XrmDatabase database)
{
    LockDisplay(display);
    /* destroy database if set up implicitly by XGetDefault() */
    if (display->db && (display->flags & XlibDisplayDfltRMDB)) {
        XrmDestroyDatabase(display->db);
        display->flags &= ~XlibDisplayDfltRMDB;
    }
    display->db = database;
    UnlockDisplay(display);
}

This is broken in Xlib since at least 2004[0], so now it's unfortunately a backwards compatibility issue...

Fixes #1256.

0: https://lists.freedesktop.org/archives/xorg-commit-diffs/2004-March/000239.html

[cdown]

Conditions for repro are:

1. Dunst starts before wm
2. Dunst is displaying a notification
3. WM starts and sends PropertyNotify

[cdown]

I was curious why others haven't run into this, and found out they have, for example this block in rxvt-unicode: https://github.com/exg/rxvt-unicode/blob/5ec6fb5ec7f0871f19814279575a2f7873e5556a/src/rxvttoolkit.C#L542-L543

[bynect]

Many thanks.

Do you know why in the rxvt code that line is protected by a #if XLIB_ILLEGAL_ACCESS ?

[cdown]

The flag is enabled by autotools if the following compiles:

Display *dpy;
dpy->xdefaults = (char *)0;

What's happened here is that the first illegal accesses they found were in xdefaults. From rxvt_display::get_resources():

  /* Get any Xserver defaults */
  if (refresh)
    {
      // fucking xlib keeps a copy of the rm string
      Atom actual_type;
      int actual_format;
      unsigned long nitems, nremaining;
      char *val = 0;

#if XLIB_ILLEGAL_ACCESS
      if (dpy->xdefaults)
        XFree (dpy->xdefaults);
#endif
[...]

They were concerned that since modifying this is not permitted by Xlib's formal API, it should be tested that it at least compiles.

Then further uses of XLIB_ILLEGAL_ACCESS were extended to other functions with broken behaviour, but the compile test was never updated to also test whether those compile (although they clearly always do, since nobody has ever complained about it).

Considering the compile test will always pass on any existing Xlib -- I looked back a long time and the relevant parts of these structs have not changed for at least 20 years) -- #if XLIB_ILLEGAL_ACCESS is always true. As such rxvt-unicode has always had this workaround enabled since its addition in January 2011.

[bynect]

Thanks for the explanation. Your solution seems reasonable to me so I'll merge 👍🏻

[cdown]

Thank you! Much appreciated.