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.
The main and the most ABI-touching thing for the envisioned 2.0 branch
is the usage of the linker-build-time allocated callsite info, avoiding
the non-economic evaluations and, under some circumstances dangerous,
heap allocations in the run-time.
Considering that v1.9.0 release (libqb.so.20) was expressly marked as
tech-preview[1,2] (hence something that shall not make it to production
use), there should be no harm for master branch (that is headed towards
2.0 and beyond) to receive noticable SONAME bump (libqb.so.100) so as to
- leave enough of space for a possible v1-compatible branch evolution
(for use cases where recompile-everything is a no-go).
in particular, with resuming with libqb.so.30, there would
be a room for 99-33 = 63 add-new-drop-nothing compatible
changes for that branch (which is more than plentiful)
- indicate some big change is going on more clearly towards client space
This is supposed to be a reasonable trade-off solution that would still
leave enough wiggle space, and would represent responsible approach to the
development (like the original attempt to prevent ABI break in the first
place was), allowing for more than an enforced unanimity (rather
antagonistic in the free software realms).
[1] https://lists.clusterlabs.org/pipermail/users/2019-December/026690.html
[2] https://github.com/ClusterLabs/libqb/releases/tag/1.9.0
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
Closes: #377
Also, for original issue, closes: #375
Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
Reviewed-by: Jan Pokorný <jpokorny@redhat.com>
Now that splint is actually contradicting errors that come from
the compilers I think it's time to retire it. I could cope with it
being a minor nuisance on the argument that "another check can't
hurt", but contradicting the actual compilers is too much.
The CI has Coverity installed which is much more up-to-date anyway.
Splint hasn't been updated since 2010
On newer Fedora systems that can have 32 bit PIDs, these long test
names can get truncated in the libqb internal buffers and thus break the
tests, so I've shortened the names.
Response structure was not initialized completely,
when mkdtemp/chown failed, server was not accepting connection yet or
connect failed for some reason.
This is not an issue, but valgrind reports this
as a problem so it is easy to miss real problem then.
Solution is to initialize response before it is used.
Signed-off-by: Jan Friesse <jfriesse@redhat.com>
this option is stricly meant for runtime debugging purposes.
do NOT use in production.
check gcc/clang man pages on how to use ASAN/UBSAN/TSAN.
Also allow to users to specificy SANITIZERS_CFLAGS and SANITIZERS_LDFLAGS
for advanced use.
Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
Previously, there were two separate logical issues:
- errno could be set negative in qb_rb_chunk_alloc when
when "reclaim" notifier failed
- _rb_chunk_reclaim (note: local scoped, hence comfortable for changes)
was already setting errno at a single (coincidentally, in a correct
way, but that'd be overwritten with the inverse because of the
previous logical issue in qb_rb_chunk_alloc), so make it set errno
at each failure path (now also when internal integrity in
_rb_chunk_reclaim failed(), sparing the callers to double on that task
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
The callers of qb_log_target_alloc() return -errno when it fails.
However, qb_log_target_alloc() wasn't setting errno.
The only failure case is when QB_TARGET_LOG_MAX (32) logs have been opened, so
it's unlikely to ever be a real-world problem. But in that case, now set errno
to EMFILE ("Too many open files").
After much discussion on IRC it was decided that 70000 iterations
of the stress patch didn't achieve anything significant over a
reasonable but smaller number. So it has been reduced to 5000 on
all platforms.
This patch also fixes a bug where test_ipc_disconnect_after_created
committed a use-after-free which could cause a crash on FreeBSD-devel.
Terminating NUL on FreeBSD is not part of the sun_path.
Add it to use sun_path as a parameter of unlink.
Signed-off-by: Jan Friesse <jfriesse@redhat.com>
In particular, qb_ipcs_rate_limit() needs to be outside the
"#ifdef HAVE_GLIB" conditional, since it gets used regardless.
This should have been like this as of 28e7259.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
We want to run every and each test we can, without reliance on
transitive deoendencies and environment "invariants".
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
Make the qbipcs.h module interdependence clear (also shedding light to
some semantic dependencies) as well.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
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>
Compared to the outer world, libqb brings rather unintuitive approach
to priorities within a native event loop (qbloop.h) -- it doesn't do
an exhaustive high-to-low priorities in a batched (clean-the-level)
manner, but rather linearly adds a possibility to pick the handling
task from the higher priority level as opposed to lower priority ones.
This has the advantage of limiting the chances of starvation and
deadlock opportunities in the incorrectly constructed SW, on the other
hand, it means that libqb is not fulfilling the architected intentions
regarding what deserves a priority truthfully, so these priorities are
worth just a hint rather than urgency-based separation.
And consequently, a discovery of these deadlocks etc. is deferred to
the (as Murphy's laws have it) least convenient moment, e.g., when
said native event loop is exchanged for other (this time priority
trully abiding, like GLib) implementation, while retaining the same
basic notion and high-level handling of priorities on libqb
side, in IPC server (service handling) context.
Hence, demonstration of such a degenerate blocking is not trivial,
and we must defer such other event loop implementation. After this
hassle, we are rewarded with a practical proof said "high-level
handling [...] in IPC server (service handling) context" contains
a bug (which we are going to subsequently fix) -- this is contrasted
with libqb's native loop implementation that works just fine even
prior that fix.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
This way, this core part can be easily reused where needed.
Note that "ready_signaller" similarity with run_ipc_server is not
accidental, following commit will justify it.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
Roles specifications are currently not applied and are rather
a preparation for the actual meaningful use to come.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
Using i7-6820HQ CPU yields these results:
Before: ~2:54
After: ~2:26
Speedup: ~16%
The main optimization lies in how run_function_in_new_process helper is
constructed, since now, there's an actual synchronization between the
parent and its child (that needs to be prioritized here, which is
furthermore help with making the parent immediately give up it's
processor possession) after the fork, so that a subsequent sleep is
completely omitted -- at worst (unlikely), additional sleep round(s)
will need to be undertaken as already arranged for (and now, just
400 ms is waited rather than excessive 1 second).
Another slight optimization is likewise in omission of sleep where
the control gets returned to once the waited for process has been
suceesfully examined post-mortem, without worries it's previous
life is still resounding.
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
There's some slight reserve for when bigger PID ranges are in use.
The method to yield the limit on prefix string was derived from
practical experience (rather than based on exact calculations).
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
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.
build test binaries at "make" or "make all" instead of "make check".
this is necessary if it´s not possible to run make check during make rpm.
Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
There were a few missed leftovers in d6875f2 regarding compatibility
with sed on FreeBSD (some commands do require a newline and/or
backslash separation).
Merges: #335
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
Proper Libs.private enables linking applications statically against
libqb: static archives (.a) don't carry their own dependency
information, unlike shared libraries (.so). Modern libc versions
include socket and RT functions, so socket_LIBS and rt_LIBS will be
empty there, but we include them for strict correctness on older
platforms; basically, we're matching libqb_la_LIBADD here.
Consequently, nsl_LIBS and GLIB_LIBS don't enter this field, since they
are only used in the examples and tests, not in the library proper.
Cflags, on the other hand, is emitted all the time and (under GCC)
propagates the -pthread option (which also affects the preprocessing
stage) to all users of libqb even when compiling modules or linking
everything dynamically.
Signed-off-by: Ferenc Wágner <wferi@debian.org>
The last fix to skiplist never ran the code that patched up the level
list as it updated the current level before runnign the loop.
This now works.
Merges: https://github.com/ClusterLabs/libqb/pull/333
Reviewed-by: Jan Pokorný <jpokorny@redhat.com>
* log: Add high-resolution timestamp option for log files
This adds the %T option to the log format for millisecond timestamps. There's a feature test macro QB_FEATURE_LOG_HIRES_TIMESTAMPS so that applications know that they are available.
Because this changes the internal logging API, applications that use custom loggers will also need to change their custom logging destinations to take a struct timespec instead of a time_t. The above feature test macro will help in deciding which is appropriate.
move from AC_PREPROC_IFELSE (strongly discouraged) to AC_COMPILE_IFELSE
our detection system was very weak and recent versions of clang did
show that PREPROC_IFELFE (cpp) would enable warning options that
the compiler does not support (clang).
use a full compilation test to detect what works and what doesn't.
Also expand the warning list to include new / renamed clang options
of equivalents already enabled for older versions of clang and gcc.
Signed-off-by: Fabio M. Di Nitto <fdinitto@redhat.com>
* log: add systemd journal as a logging option
systemd journal can be configured as a logging option
at ./configure time (--enable-systemd-journal).
If libqb is buit with this then the syslog target can be switched
to sending to the journal using
qb_log_ctl(QB_LOG_SYSLOG, QB_LOG_CONF_USE_JOURNAL, 1);
As this patch also brought up some locking issues with re-configuring the logging while threaded logging was enabled, it also includes locking around qb_log_ctl2() and conversion of in_logger to an atomic.
(Patch from poki, only committed under my name because github is being
weird)
This used to happen when an iterator contained a reference on the item
to continue with, which got outdated when such item had been removed in
the interim, though it's original memory would still be -- mistakenly --
accessed. Actually such a condition is exercised with an existing
"test_map_iter_safety(ordered=true)" test, though it likely never run
under valgrind's supervision and standard memory checking harness was
too coarse (perhaps because of low memory pressure or other "lucky"
coincidence). Thankfully, the default, paranoid approach towards dynamic
memory handling in OpenBSD (free(3) call makes small chunks "junked",
i.e., filled with 0xdf bytes, see malloc.conf(5)) resulted in the
explicit segmentation fault when tripping over the happens-to-be-freed
pointer in the assumed iteration chain.
We solve the "out-of-sync iterator" issue with a twist, inverting
the responsibility to carry (and more widely, to contribute in the
propagation of) the up-to-date "forward" pointers, as clearly,
iterating over and over through the items would not be very scalable
(and it was not done, which had resulted in the first place).
So now, when any skiplist item is to be removed, its preceding item
gets the "forward" pointers recomputed as before, but then, they are
copied into "forward" pointers for the item to be removed, original
area containing them is disposed, and this preceding item just points
to the area primarily managed by the to-be-removed item (procedure
dubbed "takeover-and-repoint" in the comment). This itself gets
a special mark so that this area won't be dropped when that item gets
disposed, which rather happens with the disposal of the preceding item
that points to the "forward" memory area at hand and is not marked so.
This is believed to be sufficient to address out-of-band (iterator
based) access versus interim future iteration chain mangling, as these
operate de facto on the non-sparse, linear level of the skiplist.
Alternative approaches include:
turning pointers-to-arrays into pointers-to-pointers-to-arrays to
allow for explicit setting to NULL after free, and sharing this
additional indirection -- this straightforward extension was
attempted first, but shortly after, it became apparent it would
be a nightmare with the current interprocedural dependencies
extra tagging of the structures and adding complexities around
checking the eligibility, like every other manipulation with the
skiplist
completely split life-cycle of "node" and "node->forward", i.e.,
separate reference-counting etc.
Also said test was extended to push the corner case to the limit:
when to-resume-with item in the chain is being figured out, the
predecessors may be consulted (it is in that test), but the very
first predecessor is now removed as well, for good measure, as
it makes for boundary condition ^ 2.
Signed-off-by: Jan Pokorný jpokorny@redhat.com
