From 3143753dbee6cab8b9364d07b6b1c439a7afa792 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Mon, 15 Jan 2018 00:02:36 +0100 Subject: [PATCH] tests: log_test_mock.sh: fix overaggressive "section populated" check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It turned out that while log_test_mock.sh already provided ways for extra variations in the composite body of the program to inspect for a working logging, "omit logging at the target client side completely, leave it along with the respective self-check just at the intermediate library this program uses" one hadn't apparently been exercised with binutils < 2.29.1 (which is the only one OK from the get-go since it arranges boundary denoting symbols for orphan sections as protected). This made the pacemaker building fail half the way with libqb 1.0.3 and binutils < 2.29.1 because the built executables are instantly run as to extract their help screen[1], and they represent the said pain pattern: - program not using libqb log subsystem directly, but - linked to dynamic library that itself does + it also utilizes QB_LOG_INIT_DATA macro, which - upon loading the executable and the the linked dependencies prior to proper run invokes the checks, where - one in particular tries to assess whether the direct-access boundary symbols are not aliasing (equal), but - because with standard visibility, symbols from the program take a precedence, actually its symbols are prioritized (instead of the ones pertaining to the library layer as expected), and - since previous fix to accommodate binutils 2.29+ fancy/new handling of the boundary symbols they are not subject to any sort of garbage collection (no symbols were emitted for a section that was not known because there were no instruction it should contain anything, until we stuck those symbols in place by force by the means of linker script to that effect), these boundary symbols indeed occur in this end program having no callsite data (see first point), meaning that start and stop address indications for the section equals, hence - the said assertion triggers (despite it should not = false positive) This clearly means that "QB_ATTR_SECTION_START != QB_ATTR_SECTION_STOP" cannot be used unconditionally. However, it would left the software using logging along with QB_LOG_INIT_DATA macro (which was already highlighted as its vital part in order to avoid silent logging malfunction) that doesn't utilize _GNU_SOURCE macro (unlocking some GNU extensions on top of basic POSIX interface) short of the main checks (because they are conditionalized for requiring said extensions), and libqb never required the client side to define that macro nor it dared to define it behind user's back as it can have unexpected consequences. Luckily, the first paragraph carries the key: protected symbols behave exactly as required here[2]: > Protected visibility is like default visibility except that it > indicates that references within the defining module bind to the > definition in that module. That is, the declared entity cannot be > overridden by another module. So we just move said comparison to "!defined(_GNU_SOURCE)" conditional branch within qblog.h, and to cater binutils prior to 2.29.1 (which has it like that by default as mentioned), we also mark the declarations of the boundary symbols, likewise conditionally, with protected visilibity (there appears to be no way for that in the linker script). But that would be too easy as once again, 2.29 linker begs to differ and these two "!defined(_GNU_SOURCE)" measures will actually do more harm than good when in the mix. Hence, new QB_LD_2_29 macro is proclaimed the kill-switch for that case, and the user becomes responsible to either define it when building with this 2.29 troublemaker (as a recap, 2.29.1 is fine from this perspective), or to start using _GNU_SOURCE. One more question mark remains, though: are not the QB_LOG_INIT_DATA's checks in _GNU_SOURCE case weaker now than used to be? The anwer is: no, as long as looking at the symbols through dlopen'd particular shared object will contain no overrides from either already loaded dependencies (if caching is so aggressive to detect it's already in loaded in caller's context) or from recursive load. Currently, it doesn't seem to be the case, but it may depend on the implementation of dynamic linking library or the toolchain. If this observation is proved to be wrong, the solution may be as simple as dropping !defined(_GNU_C) condition from the guard of making the boundary symbols with protected visibility -- it is not done now also with respect to non-gcc compilers which may not recognize that. Last but not least, dl* calls in the QB_LOG_INIT_DATA's checks are themselves subject to scrutiny now: should they fail unexpectedly, the run is terminated just as well. This led to discovery of the issue masked so far, boiling down to unability do dlopen executable (as opposed to shared object)[3]. It did not matter before, because of rather best-effort, optimistic approach (perform the final check only if precondition steps succeeded), but now, we have to add an extra stipulation that this case won't lead to premature termination -- it just happens to sometimes be the case, and there's not much we can do to detect run down on the level of the executable proactively, at least not based on the brief experiments. [1] https://github.com/ClusterLabs/libqb/pull/266#issuecomment-356855212 [2] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-visibility-function-attribute [3] https://sourceware.org/bugzilla/show_bug.cgi?id=11754 Signed-off-by: Jan Pokorný --- include/qb/qblog.h | 110 ++++++++++++++++++++++++++++++--------------- lib/libqb.pc.in | 10 ++++- lib/log.c | 2 + 3 files changed, 85 insertions(+), 37 deletions(-) diff --git a/include/qb/qblog.h b/include/qb/qblog.h index 59d56041c..5dfc4c668 100644 --- a/include/qb/qblog.h +++ b/include/qb/qblog.h @@ -1,5 +1,5 @@ /* - * Copyright 2017 Red Hat, Inc. + * Copyright 2018 Red Hat, Inc. * * All rights reserved. * @@ -83,6 +83,8 @@ extern "C" { * discern misuse of @c QB_LOG_INIT_DATA() macro in no-logging context from * broken callsite section handling assumptions owing to overboard fancy * linker -- situation that the self-check aims to detect in the first place. + * See also @ref qb_log_init_data_note_linker + * "important note on consideration when combined with binutils 2.29 linker". * * @par Configuring log targets. * A log target can be syslog, stderr, the blackbox, stdout, or a text file. @@ -298,23 +300,70 @@ typedef void (*qb_log_filter_fn)(struct qb_log_callsite * cs); #define QB_ATTR_SECTION_START_STR QB_PP_STRINGIFY(QB_ATTR_SECTION_START) #define QB_ATTR_SECTION_STOP_STR QB_PP_STRINGIFY(QB_ATTR_SECTION_STOP) -extern struct qb_log_callsite QB_ATTR_SECTION_START[]; -extern struct qb_log_callsite QB_ATTR_SECTION_STOP[]; +extern struct qb_log_callsite QB_ATTR_SECTION_START[] +#if !defined(QB_NOAPI_LOG_INT) && !defined(_GNU_SOURCE) && !defined(QB_LD_2_29) +__attribute__((visibility("protected"))) +#endif +; +extern struct qb_log_callsite QB_ATTR_SECTION_STOP[] +#if !defined(QB_NOAPI_LOG_INT) && !defined(_GNU_SOURCE) && !defined(QB_LD_2_29) +__attribute__((visibility("protected"))) +#endif +; /* Related to the next macro that is -- unlike this one -- a public API */ -#ifndef _GNU_SOURCE +#if !defined(_GNU_SOURCE) && !defined(QB_LD_2_29) +#define QB_NONAPI_LOG_INIT_DATA_EXTRA_(name) \ + { _Pragma(QB_PP_STRINGIFY(GCC warning QB_PP_STRINGIFY( \ + without "_GNU_SOURCE" defined (directly or not) \ + QB_LOG_INIT_DATA cannot check sanity of libqb proper \ + and only superficially of the target site originating \ + this check alone. \ + NOT COMPATIBLE WITH BINUTILS 2.29 (2.29.1 OK)))); \ + /* original, straightforward check */ \ + assert("target's own callsite section is populated, otherwise \ +target's build is at fault, preventing reliable logging" \ + && QB_ATTR_SECTION_START != QB_ATTR_SECTION_STOP); } +#elif !defined(_GNU_SOURCE) #define QB_NONAPI_LOG_INIT_DATA_EXTRA_(name) \ - _Pragma(QB_PP_STRINGIFY(GCC warning QB_PP_STRINGIFY( \ - without "_GNU_SOURCE" defined (directly or not) \ - QB_LOG_INIT_DATA cannot check sanity of libqb proper \ - nor of the target site originating this check alone))) + _Pragma(QB_PP_STRINGIFY(GCC warning QB_PP_STRINGIFY( \ + without "_GNU_SOURCE" defined (directly or not) \ + QB_LOG_INIT_DATA cannot check sanity of libqb proper \ + nor of the target site originating this check alone))) #else #define QB_NONAPI_LOG_INIT_DATA_EXTRA_(name) \ - { Dl_info work_dli; \ + { Dl_info work_dli; dlerror(); \ + /* sanity of the target site originating this check alone */ \ + if (dladdr(&(name), &work_dli) && (work_handle = \ + dlopen(work_dli.dli_fname, RTLD_LOCAL|RTLD_LAZY)) != NULL) { \ + work_s1 = (struct qb_log_callsite *) \ + dlsym(work_handle, QB_ATTR_SECTION_START_STR); \ + work_s2 = (struct qb_log_callsite *) \ + dlsym(work_handle, QB_ATTR_SECTION_STOP_STR); \ + assert("target's own callsite section is observable, otherwise \ +target's own linkage at fault and logging would not work reliably \ +(unless QB_LOG_INIT_DATA macro used unexpectedly in no-logging context)"\ + && work_s1 != NULL && work_s2 != NULL); \ + assert("target's own callsite section is populated, otherwise \ +target's own linkage at fault and logging would not work reliably \ +(unless QB_LOG_INIT_DATA macro used unexpectedly in no-logging context)"\ + && work_s1 != work_s2); \ + dlclose(work_handle); \ + } else if (work_s1 != QB_ATTR_SECTION_START \ + && work_s2 != QB_ATTR_SECTION_STOP) { \ + /* skip "cannot dynamically load executable" \ + https://sourceware.org/bugzilla/show_bug.cgi?id=11754 */ \ + assert((fprintf(stderr, "%s\n", dlerror()), \ + !"can detect/dlopen/dlsym target")); \ + } else { \ + assert("executable's own callsite section is populated, \ +otherwise target's build is at fault, preventing reliable logging" \ + && QB_ATTR_SECTION_START != QB_ATTR_SECTION_STOP); } \ /* libqb sanity (locating libqb by it's relatively unique \ non-functional symbols -- the two are mutually exclusive, the \ ordinarily latter was introduced by accident, the former is \ intentional -- due to possible confusion otherwise) */ \ + dlerror(); \ if ((dladdr(dlsym(RTLD_DEFAULT, "qb_ver_str"), &work_dli) \ || dladdr(dlsym(RTLD_DEFAULT, "facilitynames"), &work_dli)) \ && (work_handle = dlopen(work_dli.dli_fname, \ @@ -329,24 +378,9 @@ libqb's build is at fault, preventing reliable logging" \ assert("libqb's callsite section is populated, otherwise \ libqb's build is at fault, preventing reliable logging" \ && work_s1 != work_s2); \ - dlclose(work_handle); } \ - /* sanity of the target site originating this check alone */ \ - if (dladdr(dlsym(RTLD_DEFAULT, QB_PP_STRINGIFY(name)), &work_dli) \ - && (work_handle = dlopen(work_dli.dli_fname, \ - RTLD_LOCAL|RTLD_LAZY)) != NULL) { \ - work_s1 = (struct qb_log_callsite *) \ - dlsym(work_handle, QB_ATTR_SECTION_START_STR); \ - work_s2 = (struct qb_log_callsite *) \ - dlsym(work_handle, QB_ATTR_SECTION_STOP_STR); \ - assert("target's own callsite section observable, otherwise \ -target's own linkage at fault and logging would not work reliably \ -(unless QB_LOG_INIT_DATA macro used unexpectedly in no-logging context)"\ - && work_s1 != NULL && work_s2 != NULL); \ - assert("target's own callsite section non-empty, otherwise \ -target's own linkage at fault and logging would not work reliably \ -(unless QB_LOG_INIT_DATA macro used unexpectedly in no-logging context)"\ - && work_s1 != work_s2); \ - dlclose(work_handle); } } + dlclose(work_handle); \ + } else { assert((fprintf(stderr, "%s\n", dlerror()), \ + !"can detect/dlopen/dlsym libqb")); } } #endif /* _GNU_SOURCE */ /** @@ -373,6 +407,13 @@ target's own linkage at fault and logging would not work reliably \ * @c _GNU_SOURCE macro definition is present at compile-time means only half * of the available sanity checking will be performed, possibly resulting * in libqb's own internally logged messages being lost without warning. + * + * @anchor qb_log_init_data_note_linker + * @note When this macro is leveraged in the end executable that is moreover + * linked with ld.bfd from binutils 2.29 (2.29.1 is not of concern, + * though), make sure the respective code is compiled with at least one + * of @c _GNU_SOURCE and @c QB_LD_2_29 macros defined so as to avoid + * run-time misbehaviour of the pertaining checks. */ #define QB_LOG_INIT_DATA(name) \ void name(void); \ @@ -381,21 +422,20 @@ target's own linkage at fault and logging would not work reliably \ /* our own (target's) sanity, or possibly that of higher priority \ symbol resolution site (unless target equals end executable) \ or even the lower one if no such predecessor defines these */ \ + dlerror(); \ if ((work_handle = dlopen(NULL, RTLD_LOCAL|RTLD_LAZY)) != NULL) { \ work_s1 = (struct qb_log_callsite *) \ dlsym(work_handle, QB_ATTR_SECTION_START_STR); \ work_s2 = (struct qb_log_callsite *) \ dlsym(work_handle, QB_ATTR_SECTION_STOP_STR); \ assert("implicit callsite section is observable, otherwise \ -target's and/or libqb's build is at fault, preventing reliable logging" \ +target's and/or libqb's build is at fault, preventing reliable logging" \ && work_s1 != NULL && work_s2 != NULL); \ - dlclose(work_handle); /* perhaps overly eager thing to do */ } \ - QB_NONAPI_LOG_INIT_DATA_EXTRA_(name); \ - /* finally, original, straightforward check */ \ - assert("implicit callsite section is populated, otherwise \ -target's build is at fault, preventing reliable logging" \ - && QB_ATTR_SECTION_START != QB_ATTR_SECTION_STOP); } \ - void __attribute__ ((constructor)) name(void); + dlclose(work_handle); /* perhaps overly eager thing to do */ \ + } else { assert((fprintf(stderr, "%s\n", dlerror()), \ + !"can dlopen/dlsym main program")); } \ + QB_NONAPI_LOG_INIT_DATA_EXTRA_(name); } \ + void __attribute__((constructor)) name(void); #else #define QB_LOG_INIT_DATA(name) #endif /* QB_HAVE_ATTRIBUTE_SECTION */ diff --git a/lib/libqb.pc.in b/lib/libqb.pc.in index d9839bf41..5368604b3 100644 --- a/lib/libqb.pc.in +++ b/lib/libqb.pc.in @@ -16,8 +16,14 @@ Libs: -L${libdir} -lqb @client_dlopen_LIBS@ # possible future optimizations and extra features. Consequently, # logging issues (typically bound to QB_LOG_INIT_DATA macro) can be # mitigated with QB_KILL_ATTRIBUTE_SECTION macro defined for a build. -# NOTE: when concerned about a warning coming from the build process like +# NOTE: Conversely, when QB_LOG_INIT_DATA macro is leveraged in the end +# executable relying on this library that is moreover linked with +# ld.bfd from binutils 2.29 (2.29.1 is not of concern, though), make +# sure the respective code is compiled with at least one of _GNU_SOURCE +# and QB_LD_2_29 macros defined so as to avoid run-time misbehaviour +# of the pertaining checks. +# NOTE: When concerned about a warning coming from the build process like # warning: [...]libqb.so contains output sections; did you forget -T? -# and the build finishes OK, take it merely as a harmless side-effect +# and the build finishes OK, take it merely as a harmless side-effect. Libs.private: @LIBS@ Cflags: -I${includedir} diff --git a/lib/log.c b/lib/log.c index 1339f9185..cd1e5877b 100644 --- a/lib/log.c +++ b/lib/log.c @@ -18,6 +18,8 @@ * You should have received a copy of the GNU Lesser General Public License * along with libqb. If not, see . */ +#define QB_NOAPI_LOG_INT 1 /* preventing explicit protected visibility */ + #include "os_base.h" #include #ifdef HAVE_LINK_H