Try versioned dlopen() in libswmhack.

[andreabolognani]

On Linux (at least Debian and Fedora) .so files are part of the
development package for a library, as opposed to the runtime package.

This patch adds a second attempt at dlopen() with the major version
number added if the first one fails, so that installing the development
packages is not necessary for the hack to work.

It also prints an explicatory error message and abort()s in case
dlopen() fails instead of segfaulting without apparent reason.

[andreabolognani]

Can you please have a look at this whenever you have the time?

Thanks.

[andreabolognani]

I've refactored the code so that error checking is performed in a single spot. I've also changed it to use exit() instead of assert() when an error is detected.

[andreabolognani]

Any news at all?

[marcopeereboom]

I don't get this at all. What does the 6 mean?

[andreabolognani]

Hi,

in this case, 6 is just the current major version number of the X11 library.

On Linux, each library installed on the system (eg. ncurses 5.9) usually brings in three files:

• libncurses.so.5.9
• libncurses.so.5
• libncurses.so

the first one being the actual library and the other symbolic links to it. This allows eg. ncurses 4 and 5 to be installed side by side, because applications are linked to either libncurses.so.4 or libncurses.so.5.

Most Linux distributions, notably Debian and Fedora, only ship the .so symbolic link for a library as part of its development package (see 1, 2, 3 and 4 for libX11). Arch doesn't apparently have development packages, and as such ships everything as part of the same package (see 5).

The result of this is that most users will not have the .so link on their system, so the dlopen() call in libswmhack will fail. This pulls request introduces a second attempt at calling dlopen(), should the first one fail, this time including the major version number in the object name.

Please note that, if the .so link is available, everything works just as it did before; this change only helps people who don't have the development packages for libX11 and libXt installed. It's been included, in some form or another, in the Debian package since 2009 and no bug report has been filed because of it. Having this merged upstream would mean users of other Linux distribution will benefit from it as well.

Thank you for your time.

[andreabolognani]

Hi,

any news on this one? Was my explanation of the rationale clear enough?

Thanks.

[marcopeereboom]

This is not portable. Your linux uses that version, the rest of the world uses a different version. That is a local patch IMO.

[andreabolognani]

Hi,

it is portable to any Linux distribution I can think of, because the major number for libX11 has been 6 for almost 10 years as far as I can tell.

Besides, the patch doesn't remove the original logic: I've only added a second attempt at dlopen()ing the library, and that code is not executed at all unless the first attempt (ie. the original logic) has already failed. None of the systems where libswmhack.so worked correctly before will be affected by this change.

I could keep maintaining this as a Debian-specific patch, but I'd prefer if it was integrated upstream so that other distributions could benefit from it as well. I've just installed the Spectrwm package on Fedora 21 as a test, and I can confirm that launching any application will result in a segfault unless you explicitly unset LD_PRELOAD or install the libX11-devel package, both of which you'll agree are just ugly workarounds.

Please consider merging this pull requests.

Thanks.

[marcopeereboom]

This is on my box:
-rw-r--r-- 1 root wheel 1418388 Oct 20 20:15 /usr/X11R6/lib/libX11.so.16.0

Like I said, this is incompatible and I won't pick up that PR. Sorry.

[andreabolognani]

Is that a Linux box? What distribution are you running?

If you give me more information I can try and rework the patch so that it covers all possible scenarios. That was always the intention anyway.

Thanks.

[marcopeereboom]

That is bitrig. But this is an exercise in futility. There are N OS' out there. Spectrwm also runs on Windows and OSX. It just won't be ever accurate or complete therefore we stick with opening the "correct" version as seen by the OS.

[andreabolognani]

So that's not a Linux then. Assuming libswmhack.so is working correctly, that must mean libX11.so is part of the runtime package and not the development package on bitrig, right? My patch wouldn't break such a setup.

I don't have a Windows or OS X machine handy, so I can't confirm whether or not libX11.so.6 is available there. If libX11.so exists, though, everything would keep on working; if it doesn't exist, libswmhack.so was not working to begin with.

I guess the only way to have 100% guaranteed correct behaviour would be to inspect the executable file we're spawning, eg. xterm, the same way ldd does, and dlopen() the same library files it's linked to. That's probably beyond what I'm willing to do to have this patch merged upstream, plus I guess it would add quite a bit of runtime overhead.

I have reworked the patch again so that the value of X11_LIB_MAJOR is passed as a compiler flag at build time; it's easy for both users and maintainers alike to override the default by passing the preferred value when calling make.

The flag is only passed in the Linux Makefile, so other platforms are unaffected but still get the improved error reporting and refactored code.

Does the updated patch look good to you?

PS. A bunch of unrelated commits are now apparently part of the pull request, and I can't find a way to remove the noise. You'll find that merging will only actually touch linux/Makefile and lib/swm_hack.c.

[marcopeereboom]

I find this absolutely shocking TBH. Your linux packaging is essentially broken so as I stated before: unfortunately this has to be a distro patch.

Making it a setting is a terrible idea (security whathaveyou). The OS is supposed to take care of this; and all sane OS' seem to do it right.

[andreabolognani]

Linux packaging is broken, and the patch works around this fact. It does so on Linux only, which means sane operating systems are not affected at all. It does so upstream, because otherwise all broken operating systems that are part of the same broken family would need to be patched separately.

Care to expand a bit on the security implications?

Thanks.

[marcopeereboom]

if it is a setting "anyone" can override what is being loaded at application start time. So i can literally load whatever malicious code I'd like. I am not being mean or ugly; this has to be resolved downstream in order to fit the environment while remaining secure.

An OS is supposed to open the correct version of a library with dlopen. The spec is rather ambiguous on dlopen and essentially says "implementation-defined". That is code for OS' specified and in your particular case it means "downstream" patch.

I really am not going to allow this patch in this critical code path. I understand and am sensitive to your plight however the entire world is not a Linux distro.

[andreabolognani]

In the current implementation it's not a runtime setting, but a compiler flag that's (optionally) passed by the user (or package maintainer) when using linux/Makefile. A mailicious user has no way of overriding it once libswmhack.so has been installed.

I respect your authority as spectrwm's author to reject any patch you don't deem suitable for inclusion, so if you think my approach is wrong on a fundamental level and nothing I've done so far (eg. ensuring libX11.so is always tried first, restricting this to Linux only, making the major release easily configurable at compilation time) or I might do at a later date can make this patch acceptable, by all means go ahead and close the pull request.

I hope this conversation has at least made you aware of the fact that people using spectrwm on Linux are facing this issue, and you'll come up with a sensible solution for it.

Thanks.

[marcopeereboom]

I remain unconvinced. This is an downstream issue.