* memleak: ipc_socket: properly dispose local-scoped strndup values
Leaking memory was only possible when using filesystem sockets (see
use_filesystem_sockets function) and either:
- client is deliberately disconnecting from a server (continued run
imposes a risk of exhausting memory)
- server is deliberately disconnecting from its client (ditto, but
more substantial risk due to the common shared-resource nature
of the server)
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
* memleak: ipc_socket: properly dispose inter-function strdup values
Leaking memory was only possible when the server accepted the client,
but didn't get (or was too shy) to talk to it prior to proceeding with
a disconnect.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
* ipc_socket: care to explain what's going on with file name inference
Related to the code parts at hand, there was an investigation/fix in
the past, initiated by "make check" failure on FreeBSD 9 [rhbz#1256701].
Unfortunately, not only the magic constant being modified was not
explained in 1908e6c, but (one can derive because of a lack of solid
background of what's going on here, which might have caused that),
it was modified incorrectly at one instance (see also [PR165 comment]),
which was then reinstated in 7ebcb3d.
So, finally de-mystify those magic constants. Also break the symmetry
between the client/server further with depending on the canonical
"request socket" alias at the server side (the former worked equally but
it was unnecessarily confusing and there's a risk this artificial alias
will get removed in the future).
[rhbz#1256701] https://bugzilla.redhat.com/1256701
[PR165 comment] https://github.com/ClusterLabs/libqb/issues/165#issuecomment-142949541
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
It was not clear to non-native English speakers.
Signed-off-by: Christine Caulfield <ccaulfie@redhat.com>
Reviewed by: Jan Pokorný <jpokorny@redhat.com>
* IPC: Allow filesystem sockets to be chosen at run-time on Linux
Most of this patch came from Andrew Beekhof.
Keep a global variable that decides whether or not to use filesystem sockets
or abstract sockets for IPC connections. This variable is set by the presence of a file (default /etc/libqb/force-filesystem-sockets).
* tests: Fix test_ipcc_truncate_when_unlink_fails_shm test using FS sockets
When using filesystem sockets, the
test_ipcc_truncate_when_unlink_fails_shm test always fails, this was
because the unlink() call is wrapped to fail and so it never cleans up
the old version of the socket.
The fix is to preemptively remove the file before unlink gets wrapped.
* doc: Explain the force-filesystem-sockets option
Signed-off-by: Christine Caulfield <ccaulfie@redhat.com>
The sockets are named using a random() suffix in at attempt to isolate
concurrent test. However random() always returns the same random number
by design ... unless pre-seeded with some value being unique enough for
the particular execution.
Borrowing the most of the above message from original "srandom" fix by
Chrissie who also discovered this issue (nice!), I thought it would be
more viable if we encoded such "unique enough" variables directly to
IPC name being generated, not relying on pseudorandom generators in any
way. Hence this other fix.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
qb_loop_signal_add() used to set any signals it wasn't managing
back to SIG_DFL. This is unfriendly behaviour in a library.
Signed-off-by: Christine Caulfield <ccaulfie@redhat.com>
Reviewed-by: Jan Pokorný <jpokorny@redhat.com>
This should prevent libqb from looping in the server if the
ringbuffer gets corrupted. Instead the client will be disconnected.
Signed-off-by: Christine Caulfield <ccaulfie@redhat.com>
The intention behind enabling the syslog tests on demand (642f74d) was
to tread cautiously as libtool used to alert that something controversial
is going on:
$ ./autogen.sh && ./configure --enable-syslog-tests && make check
> ...
> *** Warning: Linking the executable log.test against the loadable module
> *** _syslog_override.so is not portable!
> ...
owing to the fact that _syslog_override.so is being linked (through
libtool) with LDFLAGS=-module.
In fact, the issue did go away with a fix to the "make install" process
(ebcff54) for which _syslog_override.so* files were being picked
undesirably prior to the fix, which manifested itself also on
"make rpm" (hence the respective commit message):
> RPM build errors:
> Installed (but unpackaged) file(s) found:
> /usr/lib64/_syslog_override.so
> /usr/lib64/_syslog_override.so.0
> /usr/lib64/_syslog_override.so.0.0.0
The explanation for the secondary positive effect on the mentioned
libtool's warning going away is rather simple:
"lib" prefix switches the expectations about the result as a library
(also due to "libdir" destination) rather than as an executable object.
Change like that ensures the result is what libtool's manual calls
"dynamic module that can be opened by lt_dlopen" rather than "runtime
library that cannot" in the context of Mac OS X, which was likely the
cause of the portability warning.
(https://www.gnu.org/software/libtool/manual/libtool.html#FOOT10)
And because the tests themselves are run through the libtool wrapper
scripts capable of relinking and other magic
What libtool manual has to say on this topic:
> Note that libtool modules don’t need to have a "lib" prefix. However,
> Automake 1.4 or higher is required to build such modules.
referring to version released 15+ years back, which is assumed anyway.
(https://www.gnu.org/software/libtool/manual/libtool.html#Modules-for-libltdl)
* * *
That being said, there's now no longer a reason to have "syslog-tests"
enablement conditionalized (no longer supposed intrusive), hence enable
them unconditionally along the other libcheck-based tests
-- whenever libcheck is detected.
This changeset also drops any trace of --enable-syslog-tests switch
in various places using that (spec file, Travis CI configuration)
making for a complete vaporizatio of this choice.
This reduces overlinking of qb-blackbox. Being a seldom used executable,
the gains are mostly theoretical, but at least this silences warnings
from some QA tools.
Unfortunately, debug messages of GNU make are emitted to stdout, which
spoils the extracted output we rely on. So prevent it (as well as
any other extraneous option) by force.
(discovered during git -> automatic COPR builds integration)
There are not many ways to test alternate code paths having failure of
some function from standard library as a precondition.
For a starter, we need to test failing unlink{,at} functions in a
controlled manner to mimic client and server path of the IPC connection
having different privileges to validate the previous commit. But the
test suite cannot assume it has root privileges (so as to add artificial
user system-wide, which is a pretty stupid idea on its own), cannot
generally use stuff like chroot/namespacing (not to speak about
synergies of the former like docker). So what's left is to make our
own playground, or better yet, use existing playground but just to
modify the rules of the game a bit when it's desired -- a variation
of old good LD_PRELOAD trick.
Note that this concept was already used in syslog tests (see commit
642f74d) and is now further extended using dlsym(RTLD_NEXT, "symbol")
to resolve the standard library symbol being shadowed by our little
"module". This hence yields a customized wrapping we use to either
inject a call failure or to increase an invocation counter so as to
assure something has indeed been called. As the mechanisms used are
not supposed to be available everywhere, the build system is
conditionalized respectively.
Back to our test when unlink{,at} fails, with the help of the described
mechanism, it was actually easy to massage test_ipc_server_fail_shm
into test_ipcc_truncate_when_unlink_fails_shm desired addition, which
is also featured in this commit, together with a modification to
resources.test script so that it expects particular number of empty
file leftovers (see previous commit).
It's expected that the module for failure injections will keep growing
so as to enable better overall coverage of the code (on the platforms
where this provision is available).
This changeset builds on previous 2-3 commits and represents the main
libqb's answer to the original question behind pacemaker's security
defect known as CVE-2016-7035.
Beside the helper partly unifying handling of qb_rb_force_close and
qb_rb_close, it provides the former with ability to use file truncating
as a fallback for when unlinking fails, e.g., because client (note that
mentioned is currently only relevant for the client side as normally
server is responsible for the lifecycle of the materialized files,
unless it crashes and only client is left to do its best) is not the
owner while they are placed at a directory with restricted deletion,
which enforces this very ownership condition.
In practice, this means that, at worst, just the zero-size files are
left behind, so not that much space exhaustion (usually "ramdisk"
like tmpfs is what backs default storage directory /dev/shm, so it
boils down to physical memory exhaustion, even if it can be just
for page cache and related overhead) can happen even on repeated
crashes as the memory mappings are cleared as much as possible.
Also openat/unlinkat functions (sported in qb_sys_unlink_or_truncate_at
as of the previous commit) are, when applicable, used so as to limit
possible race conditions between/during individual path traversals
(both files being got rid of presumably share the same directory).
Few words on which actions are attempted in which order for the
equivalent of qb_rb_force_close now:
There are subtle interactions between what's externally visible
(files) and what's not (memory mappings associated with such files),
and perhaps between memory pages management from the perspective of
the former (usually "ramdisk"/tmpfs) and the latter (mmap + munmap).
If the associated file is no longer publicly exposed by the means of
unlink (even if the object survives internally as refcounting is in
the game, with mmap holding a reference), memory mapping is not
affected. On the other hand, if it's just limited by truncation
to zero size, memory mapping is aware and generates SIGBUS in response
to accessing respective addresses. Similarly, accessing munmap'd
(no refcounting here) memory generates SIGSEGV. For delicacy,
the inputs for all of unlink, truncate, and munmap are stored
at the mmap'd location we are about to drop, but that's just a matter
of making copies ahead of time.
At Ken's suggestion, the scheme is: (unlink or truncate) then munmap,
which has a benefit that externally visible (and program's life span
otherwise surviving!) part is eliminated first, with memory mappings
(disposed at program termination automatically at latest) to follow.
(There was originally a paranoid expectation on my side that truncate
on tmpfs actually does silent munmap, so that our munmap could in fact
tear down the mapping added in the interim by the libraries, signal
handler or due to requirements of another thread, also because of
munmap on the range without any current mappings will not fail, and
thus there's likely no portable way to non-intrusively check the
status, but also due to documented SIGBUS vs. SIGSEGV differences
the whole assumption appears bogus on the second thought.)
Relevant unit tests that exercise client-side unlinking:
- check_ipc: test_ipc_server_fail_shm, test_ipc_exit_shm
- new test in a subsequent commit
These are intended for subsequent qb_rb_{force_,}close refactorization
and utilization of this new truncate as a fallback after unlink failure
as detailed in the commit to follow.
For newer POSIX revision compliant systems, there's "at" variant using
openat/unlinkat functions so that paths do not have to be traversed
in full anew when not needed (as both unlink and truncate operate on
the same path).
...using a new private inline helper that is intended to "decorate"
argument (plus extra reference level added) to qb_rb_{force_,}close().
It is purposefully not hardwired to neither qb_rb_close (it's a public
API function that should not change its semantics) nor qb_rb_force_close
(just for symmetry, preempting issues when the two would differ, and
also makes them more mutually compatible, which is already expected
at qb_ipcc_shm_disconnect).
It sets the original ringbuffer pointer to NULL (having the immediate
impact on other threads/asynchronous handling) and also sets the
(currently underused) reference counter set to exacly 1 (that is
subsequently going to be decremented in qb_rb_close so that it's
sound in the current arrangement).
More in the comment at the helper.
Suitable places are also made to use it right away.
This fixes issue with would-fail-if-applied-to-thread-right-away
qb_log_thread_priority_set invocation when logging thread doesn't
exist yet, which will arrange for calling itself at the time of
thread's birth that is the moment it will actually fail.
In this + lock-could-not-have-been-initialized corner cases, the
already running thread would proceed as allowed by error condition
handling in the main thread, trying to dereference uninitialized
(or outdated) pointer to the lock at hand, resulting in segfault.
Also include the test that would have been caught that (we use the
fact that it doesn't matter whether setting of the scheduler parameters
fails due to bad input or just because of lack of privileges as it's
the failure at the right moment that is of our interest).
See also:
https://github.com/ClusterLabs/libqb/issues/229
