Branch: refs/heads/master
Home:
https://github.com/ClusterLabs/libqb
Commit: c5aaea9207f8508c4e46479900e035938501ab82
https://github.com/ClusterLabs/libqb/commit/c5aaea9207f8508c4e46479900e03...
Author: Jan Pokorný <jpokorny(a)redhat.com>
Date: 2016-11-04 (Fri, 04 Nov 2016)
Changed paths:
M lib/ipc_shm.c
Log Message:
-----------
Refactor: ipc_shm: better grip on ringbuffers to close
Also remove unused comment-introduced section of code.
Commit: 189ca28db91f2af3a2641362ff85aa6c8960f999
https://github.com/ClusterLabs/libqb/commit/189ca28db91f2af3a2641362ff85a...
Author: Jan Pokorný <jpokorny(a)redhat.com>
Date: 2016-11-04 (Fri, 04 Nov 2016)
Changed paths:
M lib/ipc_shm.c
M lib/log_blackbox.c
M lib/ringbuffer_int.h
Log Message:
-----------
Med: rb: make it more robust against trivial IPC API misuses
...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.
Commit: 7286215ec7bb2a4bf5d26e90d87e509f958cdc0a
https://github.com/ClusterLabs/libqb/commit/7286215ec7bb2a4bf5d26e90d87e5...
Author: Jan Pokorný <jpokorny(a)redhat.com>
Date: 2016-11-04 (Fri, 04 Nov 2016)
Changed paths:
M configure.ac
M lib/unix.c
M lib/util_int.h
Log Message:
-----------
Low: unix: new qb_sys_unlink_or_truncate{,_at} helpers
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).
Commit: 15591922346ad485eb054f4c611b226dc6315393
https://github.com/ClusterLabs/libqb/commit/15591922346ad485eb054f4c611b2...
Author: Jan Pokorný <jpokorny(a)redhat.com>
Date: 2016-11-04 (Fri, 04 Nov 2016)
Changed paths:
M lib/ringbuffer.c
M lib/ringbuffer_helper.c
M lib/ringbuffer_int.h
Log Message:
-----------
Med: rb: use new qb_rb_close_helper able to resort to file truncating
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
Commit: f610b1b161950f37e1a43cb927a3d956e8738fec
https://github.com/ClusterLabs/libqb/commit/f610b1b161950f37e1a43cb927a3d...
Author: Jan Pokorný <jpokorny(a)redhat.com>
Date: 2016-11-04 (Fri, 04 Nov 2016)
Changed paths:
M configure.ac
M tests/Makefile.am
A tests/_failure_injection.c
A tests/_failure_injection.h
M tests/check_ipc.c
M tests/resources.test
Log Message:
-----------
tests: start stdlib failures injection effort with unlink{,at} + test
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).
Commit: b67f8ff9f46c40aa03c8dd33de1bab83bc92fd68
https://github.com/ClusterLabs/libqb/commit/b67f8ff9f46c40aa03c8dd33de1ba...
Author: Chrissie Caulfield <ccaulfie(a)redhat.com>
Date: 2016-11-08 (Tue, 08 Nov 2016)
Changed paths:
M configure.ac
M lib/ipc_shm.c
M lib/log_blackbox.c
M lib/ringbuffer.c
M lib/ringbuffer_helper.c
M lib/ringbuffer_int.h
M lib/unix.c
M lib/util_int.h
M tests/Makefile.am
A tests/_failure_injection.c
A tests/_failure_injection.h
M tests/check_ipc.c
M tests/resources.test
Log Message:
-----------
Merge pull request #231 from jnpkrn/unlink-or-truncate
Unlink or truncate (as a fallback) files when shm IPC client terminates connection
forcibly
Compare:
https://github.com/ClusterLabs/libqb/compare/026aaa7bdecd...b67f8ff9f46c