When qb_ipcs_connection_auth_set() has been used, the ownership of the
temp directory initially set by handle_new_connection() must be updated
as well.
It's misleading towards a random code observer, at least,
hiding the fact that what failed is actually the queing up
of some handling to perform asynchronously in the future,
rather than invoking it synchronously right away.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
It turns out that while 7f56f58 allowed for less blocking (thus
throughput increasing) initial handling of connections from clients
within the abstract (out-of-libqb managed) event loop, it unfortunately
subscribes itself back to such polling mechanism for UNIX-socket-check
with a default priority, which can be lower than desired (via explicit
qb_ipcs_request_rate_limit() configuration) for particular channel
(amongst attention-competing siblings in the pool, the term here
refers to associated communication, that is, both server and
on-server abstraction for particular clients). And priority-based
discrepancies are not forgiven in true priority abiding systems
(that is, unlikele with libqb's native event loop harness as detailed
in the previous commit, for which this would be soft-torelated hence
the problem would not be spotted in the first place -- but that's
expliicitly excluded from further discussion).
On top of that, it violates the natural assumption that once (single
threaded, which is imposed by libqb, at least between initial accept()
and after-said-UNIX-socket-check) server accepts the connection, it
shall rather take care of serving it (at least within stated initial
scope of client connection life cycle) rather than be rushing to accept
new ones -- which is exactly what used to happen previously once the
library user set the effectively priority in the abstract poll
above the default one.
It's conceivable, just as with the former case of attention-competing
siblings with higher priority whereby they could _infinitely_ live on
at the expense of starving the client in the initial handling phase
(authentication) despite the library user's as-high-as-siblings
intention (for using the default priority for that unconditionally
instead, which we address here), the dead lock is imminent also in
this latter accept-to-client-authentication-handling case as well
if there's an _unlimited_ fast-paced arrival queue (well, limited
by with number of allowable open descriptors within the system,
but for the Linux built-in maximum of 1M, there may be no practical
difference, at least for time-sensitive applications).
The only hope then is that such dead-locks are rather theoretical,
since a "spontaneous" constant stream of either communication on
unrelated, higher-prio sibling channels, or of new connection arrivals
can as well testify the poor design of the libqb's IPC application.
That being said, unconditional default priority in the isolated
context of initial server-side client authentication is clearly
a bug, but such application shall apply appropriate rate-limiting
measures (exactly on priority basis) to handle unexpected flux
nonetheless.
The fix makes test_ipc_dispatch_*_glib_prio_deadlock_provoke tests pass.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
It's hard to predict the length of formatted output, so we'd better
notice (and abort) if the description is truncated. Incidentally,
mkdtemp() does this for us in the shared memory branch, but do an
explicit check there as well for consistency, and get rid of the wrongly
parametrized strncat() risking a buffer overflow (CONNECTION_DESCRIPTION
is not the length of the source "/qb").
Similar truncation checks should be added to qb_ipcs_{shm,us}_connect()
where they build the request/response names, and possibly to other
places using snprintf().
Use mkdtemp makes sure that IPC files are only visible to the
owning (client) process and do not use predictable names outside
of that.
This is not meant to be the last word on the subject, it's mainly a
simple way of making the current libqb more secure. Importantly, it's
backwards compatible with an old server.
It calls rmdir on the directory created by mkdtemp way too often, but
it seems to be the only way to be sure that things get cleaned up on
the various types of server/client exit. I'm sure we can come up with
something tidier for master but I hope this, or something similar, will
be OK for 1.0.x.
Looks like these are not accepted with splint checker. Also fix some
other minor type -- print format specifier discrepancies.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
... also to make it the documentation refer to the implementation limits
properly.
When at it, also document some nits on the implementation side, unify
qbarray.h with project's doxygen conventions a bit + fix a typo there.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
In case of hdb.c, the problem is that `check` public API (qb_hdb_handle
member struct) item (which should not be exposed publicly like this
in the first place!) is typed as `int32_t`, whereas it was to be
compared to `uint32_t` implementation-possessed local variables
(presumably derived from the same source), which made the compiler
upset (even though there was no real reason, integer promotion to
unsigned type would happily occur, which is furthermore expected to
be fully defined as these values come from `random` that shall
return non-negative integers below `INT32_MAX`).
Hence:
- type these local variables to `int32_t` just as well, which allows to
- simplify `random` return value handling, since they are expected to be
zero-or-greater and the previously extra tested all-bits-on pattern
makes undoubtfully for a negative numeric value in case of a signed
integer with specified width (c.f. 7.18.1.1/C99), hence falling into
complement of zero-or-greater; zero itself is also excluded for the
reasons stated in the comment (which was pretty hazy and incorrect,
so it gets overhaul as well)
- also superfluous typecasts are removed
Similar situation is with loop_timerlist.c, where we are actually fully
in charge of the struct member (private API), but there are good reasons
to stay consistent with the former file as the same applies to the
source of that value -- it comes from `random` (equivalent comment
is added here for greater symmetry).
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
The object in question has never been published through the header file,
hence it's presumably safe to make it static as it's meant to be.
On the other hand, QB_LOG_INIT_DATA macro from qblog.h has already
started to depend on that symbol so as to locate the library handle
for libqb itself correctly. This is trivially fixed by finally exposing
library versioning info in run-time ("online") as a structure with
members corresponding to compile-time ("offline") counterparts from
qbconfig.h header file, which are admittedly of very limited use
as opposed to the newly introduced dynamic info, plus lower-cased
equivalent of QB_VER_STR. Better than to roll out a futile data object
serving as an artificial anchor for the above purpose, and this was
due for a while, afterall.
In turn, also bump "current" and "age" of fields of the libtool's
"-version-info" versioning system.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
(or rather [read on]: "bare" fix, now that we established means to
analyse the impact of the linker-dependent misbehaviour and to detect
some of its symptoms in preceding two commits, respectively)
Initially with the help of the internal test suite and the failing log
test, it was eventually discovered[1] that these binutils commits going
to the recent 2.29 release affected the treatment of _start_SECNAME
and __stop_SECNAME symbols denoting the boundary start/stop addresses
of a SECNAME orphan section -- specifically in libqb context a custom
section (SECNAME=__verbose) used for link-time ("run-time amortizing")
callsite collection when there's a support in the toolchain[*]:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cbd0eecf261c2447781f8c89b0d955ee66fae7e9https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b27685f2016c510d03ac9a64f7b04ce8efcf95c4https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7dba9362c172f1073487536eb137feb2da30b0ff
The first one explicitly states:
> Also __start_SECNAME and __stop_SECNAME symbols are marked as hidden
> by ELF linker so that __start_SECNAME and __stop_SECNAME symbols for
> section SECNAME in different modules are unique.
The problem is that libqb silently depends on the previous status quo
ld.bfd linker behaviour of keeping those symbols externally visible,
which was apparently not granted as it has deliberately changed per
above.
And then for 2.29.1 release of binutils once again, as someone actually
noticed something went overboard with the 2.29 changes:
http://lists.gnu.org/archive/html/bug-binutils/2017-08/msg00195.html
(overview of the original bug discussion, rather than directly
https://sourceware.org/bugzilla/show_bug.cgi?id=21964, which is
a result of a conflct resolution when restoring bugzilla backup)
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=487b6440dad57440939fab7afdd84a218b612796
At least that change doesn't invalidate all the effort being put into
the original version of the changeset, only the configure script check
had to be refined so as not to miss the "orphan section magic not
working properly out of the box, without band aid" observation
(see the inline comment) -- the workaround arrangement needs
to be applied in that case as well.
* * *
So regarding the solution itself, the core of the fix was sketched at
the original Fedora targeted bug against binutils[2]. In short, we are
using a custom linker script that (re)describes the mentioned custom
orphan output section, or better yet, assuredly pushes that section, and
more importantly, it's own boundary denoting symbols, through into the
resulting executable when it's being linked (as in compile-time step).
This solution alone, while working for the non-libqb (more on that
below) logging participants, is not good enough, as it requires all
libqb targets to start using new incantation (namely "-Wl,foo.t" switch)
in the final link step during compilation, which might be solvable
with a tweak in libqb's pkg-config file under assumption that practice
of using "pkg-config --libs libqb" is rigidly followed. Which is likely
a false expectation, and furthermore only for the regular consumption
model, as it doesn't cover the least bit the developmental one (refer
to previous-but-one "tests" commit message), e.g. applied for internal
examples + tests (but no local sub-checkout tree usage can be excluded).
So further extensions were devised to cover both consumption models:
- a. regular:
courtesy of binutils maintainer[3], we follow an idea to make libqb.so
(i.e. what the targets link against) rather a linker script on its
own, which first include the version-specified (e.g. libqb.so.0) file
into the link, then lists, in situ, the content of the linker script
per above, hence -lqb linking has the same effect as having both
"-lqb -Wl,foo.t" explicitly in the link command prior to this trick
- b. developmental:
to eliminate any kind of race condition arising from the attempt
to post-modify libqb.la libtool archive file generated internally
by libtool, we sort of abuse "inherited_linker_flags" variable
within this file format, as it forms an accumulative value across
the whole transitive dependencies chain (if not impaired per the
note below), fitting exactly our purpose of injecting "-Wl,foo.t"
switch equivalent for those libtool-linking by L{D,IB}ADD'ing
libqb.la; it's then enough to craft a custom libtool archive file
declaring that value, and hook it into such dependency chain through
libqb_la_LIBADD, and with a little bit of further fiddling, it works
as desired (note that double occurrence of "-Wl,foo.t" equivalent
present at some stages of sorting this trick turned out to be,
surprisingly, counter-productive, which should now demistify the
very existence of effectively empty qblog_script_noop.ld file);
NOTE: some forms of libtool distribution (debian + derivatives ones
in particular) undermine natural transitive dependency propagation
with a deliberate cut off (https://bugs.debian.org/702737), so we
need to ensure the "impairment" is not happening by force (corosync
precedent: https://github.com/corosync/corosync/commit/0f1dc5c1)
^ something like this needs to be applied for any such "private
consumer" (although it hopefully goes without saying this way
of consuming libqb outside of it's own playground is hardly
the Right Thing) if portability is important, nonetheless!
* * *
On the address of linker script workaround, there are linkers out there
that do not support the trick, for instance:
- ld.gold from binutils (but it has hardly ever been working with
orphan sections, anyway:
https://sourceware.org/bugzilla/show_bug.cgi?id=22291)
- ancient versions of ld.bfd, e.g. 10+ years old one used as a native
system linker even in the most recent releases of FreeBSD, unless
GCC toolchain is used instead
If these are hit when (because) the compiler has already demonstrated it
supports "section" attribute, the build system configuration is forcibly
stopped, simply to stay conceptually compatible with the prior state in
which the affinity to leverage that feature hasn't been called off
under any circumstances. One is, however, able to achieve exactly
this behaviour with --enable-nosection-fallback switch, but if some
other participants in the logging, possibly linked with a more friendly
linker, do utilize this orphan section, logging may silently break
(another reason to require an explicit sign-off).
Another note, the particular self-check change slightly touched in the
previous commit but otherwise predating this whole effort by far needs
to be modified now once again, this time because linker-script-based
workaround for newer linkers as stated causes the section boundary
symbols to be present regardless if that section is utilized, leading
to a self-inflicted breakage due to these empty section symbols suddenly
winning in the symbol resolution mechanism (previously the empty section
would be dropped incl. the boundary symbols), causing problems down the
line. It also makes this very check self-contained in the same
compilation unit that trigggers it, whereas previously it used to be the
said "arbitrary" winner and things kept silently working just because
failure condition -- empty section -- would be implicitly isolated.
Last but not least, libqb itself needs to be linked with the mentioned
"-Wl,foo.t" equivalent for its own outgoing log messages to be honoured
under all circumstances, which is already achieved with the arrangement
for b. above, and by experiments, further redefinition of those boundary
denoting symbols as weak was necessary so as to make them truly global
within libqb.so proper (at least with binutils 2.29).
* * *
To provide a high-level prioritized overview of what drove the approach:
- PRESERVATION OF BINARY COMPATIBILITY (ABI), which is achieved except
for a single "ABI nongracefulness" I am aware of but that's more
a consequence of slightly incorrect assumptions in the logic of
QB_LOG_INIT_DATA macro function predating this whole affair by
a long shot and which the patchset finally rectifies:
if in the run-time dynamic link, following is combined:
(. libqb, arbitrary variant: pre-/post-fix, binutils < / >= 2.29)
. an "intermediate" library (something that the end executable links
with) triggering QB_LOG_INIT_DATA macro and being built with
pre-fix libqb (and perhaps only with binutils < 2.29)
. end executable using no libqb's logging at all, but being built
with post-fix libqb (and arbitrary binutils < / >= 2.29)
then, unlike when executable is built with pre-fix libqb, the
special callsite data containing section in the ELF structure
of the executable is created + its boundary denoting symbols
defined within, despite the section being empty (did not happen
with pre-fix libqb), and because the symbols defined within the
target program have priority over that of shared libraries in the
symbol resolution fallback scheme, the assertion of QB_LOG_INIT_DATA
of the mentioned intermediate library will actually be evaluating
the inequality of boundaries for the section of the executable(!)
rather than it's own (or whatever higher prio symbols are hit,
presumably only present if the section at that level is non-empty,
basically a generalization of the story so far);
the problem then manifests as unability to run said executable
as it will fail because of the intermediate library inflicted
assertion (sadly with very unhelpful "Assertion `0' failed"
message);
fortunately, there's enough flexibility so as how to fix
this, either should be fine:
. have everything in the executable's library dependency closure
that links against libqb assurably (compile-time) linked with one
variant of libqb only (either all pre-fix or post-fix, mind the
apparent limitation of binutils' versions with the former)
. have the end executable (that does not use logging at all as
discussed precondition) linked using substitution like this:
s/-lqb/-l:libqb.so.0/ (you may need to adapt the number later)
and you may also need to add this CPPFLAG for the executable:
-DQB_KILL_ATTRIBUTE_SECTION
- as high level of isolation of the client space from the linker
(respectively toolchain) subtleties as possible (no new compilation
flags and such required, plus there's no way to hook any dynamic
computational ad-hoc decision when the compilation is about to
happen, anyway), and in turn, versatility is preserved as much as
possible
* * *
Finally, let's have a look how the already well-known test matrix
overview changes as of this commit, but first as a recap,
"X(Y)" denotes "X linked with linker Y":
X(a) .. ld.bfd < 2.29
X(b) .. ld.bfd = 2.29 (+ 2.29.1 and hopefully on)
and here you are (values in <angle brackets> denote non-trivial change
[not mere rewording] introduced as of this commit, in comparison to the
table stated in the preceding commit):
+=========+=========+=========+=========+=========+=========+=========+
#client(x)# libqb(a) usage # libqb(b) usage #
# vvv #---------+---------+---------+---------+---------+---------+
# V # direct | libX(a) : libX(b) # direct | libX(a) : libX(b) #
+=========+=========+=========+=========+=========+=========+=========+
# x = a # OK | OK : <OK> # <OK> | <OK> : <OK> #
# x = b # <OK> | <OK> : <OK> # <OK> | <OK> : <OK> #
+=========+=========+=========+=========+=========+=========+=========+
Everything is green \o/
* * *
Note: as of this fix, it is assumed that the non-green counterpart of
this table in the message for the preceding commit (loosely though[!],
as the occurrence of empty callsite section can no longer be attributed
to something bad going on as of this fix that enforces its presence
unconditionally, whereas it would be suppressed when unused before
with kind linkers, hence some other conditions can be witnessed
especially when QB_LOG_INIT_DATA misused in no-logging context)
doubles as an indicator how will mixing the logging participants wrt.
linker+libqb version work out, when "X(Y)" becomes read as "X linked
with linker Y under additional restriction on libqb version when
compile-time link is performed of the particular part":
X(a) .. ld.bfd < 2.29 OR [arbitrary ld.bfd AND libqb after this fix)
X(b) .. ld.bfd = 2.29 (and likely on) AND libqb up to, but excluding
this fix
* * *
Let's also state some imperfections and loops kept open:
Deficiencies:
* whenever anything is compiled against our install-time-modified
libqb.so so as to force the visibility of the discussed symbols
(or when compiling [with] libqb internally):
> /usr/bin/ld: warning: ../lib/qblog_script.ld contains output sections; did you forget -T?
- not solvable as long as we use the linker script, and there's
hardly any other way not requiring the libqb consumers to adapt
in any aspect
* as already mentioned, lacking compatibility with ld.gold linker and
won't foreseeably be (cf. https://bugzilla.redhat.com/1500898#c7)
- please stick with ld.bfd (i.e. default ld linker), which you
had to do in the past anyway (at least for compiling libqb
itself)
Open questions:
* should we enable attribute((__section__)) for powerpc and other minor
platforms if the feature is proved to be working there as well?
and if/when that's going to happen, we need to figure out the
transition plan to be spread throughout an extended period to keep
the transition smooth -- notably when now-with-callsite-section
clients will get run-time linked with callsite-section-not-a-default
libqb (say upon it's downgrade), and for that, the libqb's support
alone should be enabled year(s) ahead of the actual client space...
* * *
[*] basically GCC's section("SECNAME") __attribute__ annotation of the
global variables + linker described behaviour previously mistakenly
taken for granted
References:
[1] http://oss.clusterlabs.org/pipermail/developers/2017-July/000503.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1477354#c2 + comment 8
[3] https://bugzilla.redhat.com/show_bug.cgi?id=1477354#c9
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
Now that the previous commit provides a foundation for what exactly can
go wrong with ld.bfd = 2.29+ linker, let's start reconciling that with
some reasonable assurance that logging is not silently severed, because
realizing the logs are missing is otherwise bound to happen when the
logs are suddently pretty crucial analytical resource :-)
We'll proceed in two steps as detailed.
* * *
As a first step, the table below concludes how the test matrix overview
introduced with a message for the preceding commit (also introducing
log_test_mock.sh runner which got reused here) looks as of this
refreshed sanity check, once QB_LOG_INIT_DATA macro at hand gets applied
(meaning "for non-libqb logging participants" so as not complicate the
matrix further). That macro is nothing triggered directly, it will just
plant a constructor-like function (to be invoked automatically early in
the execution) that will run through the checks (one original and couple
of new ones as of this changeset).
Note that for libqb users, this implies a new link dependency on libdl,
because they may opt-in for refreshed QB_LOG_INIT_DATA sanity check that
calls out to dlopen/dlsym/dladdr directly in case of "attribute section"
being available for the particular platform, and hence immediately needs
those symbols resolved in link time. Hence, add this conditional link
dependency to libqb.pc pkg-config file under Libs variable -- we
actually restore the occurrence of "-ldl" there as it used to be present
until commit 56754d0. While doing so, also move immediate link
dependencies of libqb (if any, currently not but that may be
a regression arising from the cleanup related to the mentioned commit)
represented with the LIBS autoconf variable under Libs.private variable
in libqb.pc, where it belongs per pkg-config documentation.
The promised table follows, but first as a recap, "X(Y)" denotes
"X linked with linker Y":
X(a) .. ld.bfd < 2.29
X(b) .. ld.bfd = 2.29 (and only 2.29)
goes like this (values in <angle brackets> denote non-trivial change
[not mere rewording] introduced as of this commit, in comparison to
the table stated in the preceding commit):
+=========+=========+=========+=========+=========+=========+=========+
#client(x)# libqb(a) usage # libqb(b) usage #
# vvv #---------+---------+---------+---------+---------+---------+
# V # direct | libX(a) : libX(b) # direct | libX(a) : libX(b) #
+=========+=========+=========+=========+=========+=========+=========+
# x = a # OK | OK : BAD[*2] #<BAD[*E]>|<BAD[*F]>:<BAD[*G]>#
# x = b # BAD[*A] | BAD[*B] : BAD[*C] #<BAD[*E]>|<BAD[*F]>:<BAD[*G]>#
+=========+=========+=========+=========+=========+=========+=========+
Woefully, nothing changes if we swap binutils 2.29 for 2.29.1, i.e.,
X(b) .. ld.bfd = 2.29.1, compared to previous state, i.e., the second
table from the previous commit is still applicable for that situation.
The added sanity checks are useful nonetheless, consider for example,
that attribute-section-less libqb is what gets run-time linked to an
attribute-section-full target. The most precise check we could use
-- a custom logger function applied in a self-test scheme -- is not
available at the point the macro-defined function gets invoked, simply
because qb_log_init hasn't been invoked by the time that constructor
gets triggered. However, what we can do is to add a non-trapping
libqb-residing reverse-testing of the client space that (and once it)
voluntarily initiates qb_log_init (delivering abruption all of a sudden
at some unanticipated, as opposed to a well-timed like with
constructors, execution point, seems pretty bad idea + libqb as
a library is a mere helper, not an undertaker :) -- this check then
only announces, via syslog (the only pre-enabled logging target),
the target's logging may be severed.
* * *
Hence, as a promised second step, after incorporating the syslog
change (and extending log_test_mock.sh so as to capture syslog
stream within the container), not much changes with the table above,
i.e., X(b) .. ld.bfd = 2.29:
[*A] in addition, unless QB_LOG_INIT_DATA used on client side,
syslog carries this notice:
"(libqb) log module hasn't observed target chain supplied callsite
section, target's and/or libqb's build is at fault, preventing
reliable logging (unless qb_log_init invoked in no-custom-logging
context unexpectedly, or target chain built purposefully without
these sections)"
logged by libqb proper
[*C] in addition, unless QB_LOG_INIT_DATA used on interlib side,
syslog carries this notice:
"(libqb) log module hasn't observed target chain supplied callsite
section, target's and/or libqb's build is at fault, preventing
reliable logging (unless qb_log_init invoked in no-custom-logging
context unexpectedly, or target chain built purposefully without
these sections)"
logged by libqb proper
[*E] in addition, unless QB_LOG_INIT_DATA used on client side,
syslog carries this warning:
"(libqb) log module has observed target chain supplied section
unpopulated, target's and/or libqb's build is at fault, preventing
reliable logging (unless qb_log_init invoked in no-custom-logging
context unexpectedly)"
logged by libqb proper
[*F] in addition, unless QB_LOG_INIT_DATA used on interlib side,
syslog carries this warning:
"(libqb) log module has observed target chain supplied section
unpopulated, target's and/or libqb's build is at fault, preventing
reliable logging (unless qb_log_init invoked in no-custom-logging
context unexpectedly)"
logged by libqb proper
[*G] in addition, unless QB_LOG_INIT_DATA used on interlib side,
syslog carries this warning:
"(libqb) log module has observed target chain supplied section
unpopulated, target's and/or libqb's build is at fault, preventing
reliable logging (unless qb_log_init invoked in no-custom-logging
context unexpectedly)"
logged by libqb proper
but desirably changes with "X(b) .. ld.bfd = 2.29.1" one
(DEP ~ "depends"):
+=========+=========+=========+=========+=========+=========+=========+
#client(x)# libqb(a) usage # libqb(b) usage #
# vvv #---------+---------+---------+---------+---------+---------+
# V # direct | libX(a) : libX(b) # direct | libX(a) : libX(b) #
+=========+=========+=========+=========+=========+=========+=========+
# x = a # OK | OK : DEP[*J] #<BAD[*M]>|<BAD[*M]>:<BAD[*L]>#
# x = b #<DEP[*N]>| DEP[*I] :<DEP[*O]>#<BAD[*M]>|<BAD[*M]>:<BAD[*L]>#
+=========+=========+=========+=========+=========+=========+=========+
* * *
[*1] client logging not working
[*2] interlib logging not working
[*3] both client and interlib logging not working
[*A] boils down to [*1], unless QB_LOG_INIT_DATA used on client side,
which then fails on
"implicit callsite section is populated, otherwise target's build
is at fault, preventing reliable logging"
assertion
[*B] boils down to [*1], unless QB_LOG_INIT_DATA used on interlib side,
which then fails on
"implicit callsite section is populated, otherwise target's build
is at fault, preventing reliable logging"
assertion
[*C] boils down to [*3], unless QB_LOG_INIT_DATA used on interlib side,
which then fails on
"implicit callsite section is populated, otherwise target's build
is at fault, preventing reliable logging"
assertion
[*E] boils down to [*1], unless QB_LOG_INIT_DATA used on client side,
which then fails on
"implicit callsite section is self-observable, otherwise target's
and/or libqb's build is at fault, preventing reliable logging"
assertion
[*F] boils down to [*3], unless QB_LOG_INIT_DATA used on interlib side,
which then fails on
"libqb's callsite section is populated, otherwise libqb's build is
at fault, preventing reliable logging"
assertion
[*G] boils down to [*3], unless QB_LOG_INIT_DATA used on interlib side,
which then fails on
"implicit callsite section is self-observable, otherwise target's
and/or libqb's build is at fault, preventing reliable logging"
assertion
[*I] boils down to [*1], unless QB_LOG_INIT_DATA used on client side,
which makes it, likely through self-reference keepalive (see
below), work OK
[*J] boils down to [*2], unless QB_LOG_INIT_DATA used on interlib side,
which makes it, likely through self-reference keepalive (see
below), work OK
[*K] boils down to [*3]
in addition, syslog carries this notice:
"(libqb) log module hasn't observed target chain supplied callsite
section, target's and/or libqb's build is at fault, preventing
reliable logging (unless qb_log_init invoked in no-custom-logging
context unexpectedly, or target chain built purposefully without
these sections)"
logged by libqb proper
[*L] boils down to [*3], unless QB_LOG_INIT_DATA used on interlib side
(sufficient?), which makes it, likely through self-reference
keepalive (see below), boil down just to [*1];
in addition, syslog carries this notice:
"(libqb) log module hasn't observed target chain supplied callsite
section, target's and/or libqb's build is at fault, preventing
reliable logging (unless qb_log_init invoked in no-custom-logging
context unexpectedly, or target chain built purposefully without
these sections)"
logged by libqb proper
[*M] boils down to [*1];
in addition, syslog carries this notice:
"(libqb) log module hasn't observed target chain supplied callsite
section, target's and/or libqb's build is at fault, preventing
reliable logging (unless qb_log_init invoked in no-custom-logging
context unexpectedly, or target chain built purposefully without
these sections)"
logged by libqb proper
[*N] boils down to [*M], unless QB_LOG_INIT_DATA used on client side,
which makes it, likely through self-reference keepalive (see
below), work OK
[*O] boils down to [*K], unless QB_LOG_INIT_DATA used on both client
and interlib side, which makes it, likely through self-reference
keepalive (see below), work OK (it's expected that this a mere
composite of situations [*I] and [*J] with consequences as stated)
* * *
Note: the only problematic (i.e. not captured automatically by the
QB_LOG_INIT_DATA macro presumably utilized at every non-libqb logging
system participant in the form of a discrete compilation unit)
combination with 2.29 is the one intersecting at "BAD[*2]" pertaining
"everything but interlib compiled with ld.bfd < 2.29". It would, of
course, be solvable as well, but presumably not in an easy way, and
that use case should not be as frequent.
Takeway: whenever your target (library or client program) actively
utilizes logging (meaning it emits at least a single log message,
otherwise there's an imminent danger of possibly even run-terminating
false positive in the self-check mechanism!),
YOU ARE strongly ENCOURAGED TO USE QB_LOG_INIT_DATA macro function
at (exactly) one of the source code files (presumably the main one)
per respective target's compilation unit.
It will alleviate the hassles possibly caused by downgrading libqb
to the linker-vs-libqb incompatibly compiled one or in similar
circumstances arising merely from the linker behaviour change,
which the current build system/code shake is all about.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
Using years-old benchmark attached to PostreSQL ML[1], I've observed
tiny bit more than double boost in speed when using fdatasync instead of
traditional fsync, on two Linux machines, each equipped with an SSD.
While the observation may be disputable (there are various
interpretations to what "synchronized I/O" actually means), by logical
extension of what the two are supposed to do, one can expect fdatasync
will perform no worse than fsync. Having the timestamps correct is
really not a priority, compared to timely processing of the message
stream. So let's use it whenever possible with QB_LOG_CONF_FILE_SYNC
requested.
As an aside, PostreSQL seems to be be using "fdatasync" method of
updating out to disk by default on Linux till today[2].
[1] https://www.postgresql.org/message-id/1095055866.414539fadb90d@webmail.rawbw.comhttps://www.postgresql.org/message-id/attachment/20659/syncbench.c
[2] https://www.postgresql.org/docs/current/static/runtime-config-wal.html#GUC-WAL-SYNC-METHOD
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
* Low: log: prevent static vs. implicit non-static declaration clash
...of qb_log_callsites_dump_sect, that could happen when its usage
in qb_log_callsites_register was uncommented.
* Low: tests: fix duplicate "const" declaration specifier
This is a follow-up for d69cc7b (making the pointer itself constant was
meant as a self-defense, no-overwrite measure).
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
* Fix typo: p{rr -> r}ocess
* doc: qbrb.h: several fixes to punctuation
* doc: qbrb.h: reindent example writer's error label as per reader
* doc: qbhdb.h: explain former importance to libqb itself
* doc: ipcc.c: explain why client would timebox recv from server
Also refer to commit d633b4e.
* doc: qblog.h: minor stylistic/doxygen markup cosmetics
* doc: qblog.h: note qb_log_format_set vs. fork interaction wrt. PIDs
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
The instances of the former in a private static variable name, the
latter in the comment to an internal-use-only function-like construct.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
* 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>
* 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>
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>
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.
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.
