From 30dcfa3276c83039973b1af5ea9eb00e8842bca2 Mon Sep 17 00:00:00 2001 From: Michal Bloch Date: Fri, 22 Feb 2019 17:23:18 +0100 Subject: [PATCH] libdlog: readd the destructor Change-Id: I0fdd0e93d55e7d8055b50600378972c0e11e685a Signed-off-by: Michal Bloch --- Makefile.am | 2 +- include/libdlog.h | 3 ++ src/libdlog/log.c | 79 +++++++++++++++++++++++++++++++++++---------- src/libdlog/log_android.c | 11 +++++++ src/libdlog/log_pipe.c | 14 ++++++++ src/tests/libdlog_android.c | 6 ++++ src/tests/libdlog_pipe.c | 17 +++++++++- 7 files changed, 113 insertions(+), 19 deletions(-) diff --git a/Makefile.am b/Makefile.am index 1476cc1..48e5137 100644 --- a/Makefile.am +++ b/Makefile.am @@ -242,7 +242,7 @@ src_tests_fdi_logger_LDFLAGS = $(AM_LDFLAGS) -Wl,--wrap=logger_open_buffer_from_ src_tests_libdlog_pipe_SOURCES = src/tests/libdlog_pipe.c src/libdlog/log_pipe.c src/shared/logcommon.c src/shared/logconfig.c src/shared/queued_entry.c src/shared/translate_syslog.c src/shared/parsers.c src_tests_libdlog_pipe_CFLAGS = $(check_CFLAGS) -pthread -src_tests_libdlog_pipe_LDFLAGS = $(AM_LDFLAGS) -lpthread -Wl,--wrap=syslog_critical_failure,--wrap=connect,--wrap=write,--wrap=recv_pipe,--wrap=dup2,--wrap=socket -lm +src_tests_libdlog_pipe_LDFLAGS = $(AM_LDFLAGS) -lpthread -Wl,--wrap=syslog_critical_failure,--wrap=connect,--wrap=write,--wrap=recv_pipe,--wrap=dup2,--wrap=socket,--wrap=close -lm src_tests_libdlog_android_SOURCES = src/tests/libdlog_android.c src/libdlog/log_android.c src/shared/logcommon.c src/shared/parsers.c src_tests_libdlog_android_CFLAGS = $(check_CFLAGS) diff --git a/include/libdlog.h b/include/libdlog.h index 69f0476..39ac016 100644 --- a/include/libdlog.h +++ b/include/libdlog.h @@ -12,6 +12,8 @@ struct log_config; extern int (*write_to_log)(log_id_t, log_priority, const char *tag, const char *msg); +extern void (*destroy_backend)(); + extern pthread_rwlock_t log_limiter_lock; extern bool limiter; @@ -19,5 +21,6 @@ void __update_plog(const struct log_config *conf); #ifdef UNIT_TEST void __dlog_fini(void); +void __dlog_destroy(void); void __configure(void); #endif diff --git a/src/libdlog/log.c b/src/libdlog/log.c index 7ded48c..ad785e5 100644 --- a/src/libdlog/log.c +++ b/src/libdlog/log.c @@ -49,7 +49,11 @@ * @see __dlog_init_backend */ int (*write_to_log)(log_id_t log_id, log_priority prio, const char *tag, const char *msg) = NULL; +void (*destroy_backend)(); + pthread_rwlock_t log_limiter_lock = PTHREAD_RWLOCK_INITIALIZER; +static pthread_rwlock_t log_destruction_lock = PTHREAD_RWLOCK_INITIALIZER; + extern void __dlog_init_pipe(const struct log_config *conf); extern void __dlog_init_android(const struct log_config *conf); @@ -238,29 +242,32 @@ static int dlog_check_limiter(log_id_t log_id, int prio, const char *tag) return DLOG_ERROR_NONE; } -static int __write_to_log(log_id_t log_id, int prio, const char *tag, const char *fmt, va_list ap, bool check_should_log) +static int __write_to_log_critical_section(log_id_t log_id, int prio, const char *tag, const char *fmt, va_list ap, bool check_should_log) { + if ((check_should_log || limiter_apply_to_all_buffers) && (dlog_check_limiter(log_id, prio, tag) < 0)) + return DLOG_ERROR_NONE; + char buf[LOG_MAX_PAYLOAD_SIZE]; + vsnprintf(buf, sizeof buf, fmt, ap); + return write_to_log(log_id, prio, tag, buf); +} +static int __write_to_log(log_id_t log_id, int prio, const char *tag, const char *fmt, va_list ap, bool check_should_log) +{ int ret = dlog_check_validity(log_id, prio, tag); if (ret < 0) return ret; - if (!write_to_log) - return DLOG_ERROR_NOT_PERMITTED; - - /* if limiter_apply_to_all_buffers config variable is set to 1, - * check_should_log value does not matter and the entry is always - * tested against all conditions, i.e. limiter rules - */ - ret = (check_should_log || limiter_apply_to_all_buffers) ? dlog_check_limiter(log_id, prio, tag) : 0; - - if (ret < 0) - return DLOG_ERROR_NONE; - - vsnprintf(buf, sizeof buf, fmt, ap); + /* The only thing that needs to be protected here is `write_to_log` since + * all other resources already have their own specific locks (and even the + * pointer could be made to point at a null handler instead of a true NULL) + * but giving this guarantee makes everything a lot simpler as it removes + * the risk of something suddenly becoming NULL during processing. */ + pthread_rwlock_rdlock(&log_destruction_lock); + ret = write_to_log ? __write_to_log_critical_section(log_id, prio, tag, fmt, ap, check_should_log) : DLOG_ERROR_NOT_PERMITTED; + pthread_rwlock_unlock(&log_destruction_lock); - return write_to_log(log_id, prio, tag, buf); + return ret; } /** @@ -317,16 +324,54 @@ int dlog_print(log_priority prio, const char *tag, const char *fmt, ...) return ret; } -#ifdef UNIT_TEST /** * @brief Finalize DLog * @details Finalizes and deallocates the library - * @notes Used in tests only + * @notes Used directly in tests; brings back the pre-init state */ +#ifndef UNIT_TEST +static +#endif void __dlog_fini(void) { + if (destroy_backend) { + destroy_backend(); + destroy_backend = NULL; + } + write_to_log = NULL; + __log_limiter_destroy(); __dynamic_config_destroy(); } + +#ifdef UNIT_TEST +void +#else +static void __attribute__((destructor)) #endif +__dlog_destroy(void) +{ + pthread_rwlock_wrlock(&log_destruction_lock); + __dlog_fini(); + + /* IMPORTANT! The lock is NEVER RELEASED. This is done ON PURPOSE. + * The critical section can still be reached in some ways, NONE LEGAL. + * + * The first is that the program links the library dynamically and keeps + * pointers to API functions past dlclose(). This is UNDEFINED BEHAVIOUR + * and the implementation is NOT REQUIRED to keep the functions in memory + * AT ALL and doing this COULD HAVE ALREADY CRASHED the program under a + * different implementation. + * + * The second is when linking statically and FAILING TO JOIN threads before + * exit(). These threads then typically keep running and can access the + * library interface. However, they WILL DIE ANY MOMENT NOW ANYWAY so + * getting deadlocked is of no consequence. + * + * In theory it would be possible to detect that destruction has already + * taken place and reinitialize the library to salvage a logging attempt. + * This is a HORRIBLE IDEA since without a destructor to rely on, either + * those RESOURCES WOULD LEAK or we would have to MASSIVELY CONVOLUTE our + * logic for manual destruction in such cases. */ +} diff --git a/src/libdlog/log_android.c b/src/libdlog/log_android.c index 6da740b..d86f18e 100644 --- a/src/libdlog/log_android.c +++ b/src/libdlog/log_android.c @@ -74,6 +74,16 @@ static int __write_to_log_android(log_id_t log_id, log_priority prio, const char return ret; } +static void __destroy_androidlogger() +{ + /* No need for an explicit lock here; this should only be called + * by the library destructor which has its own lock */ + + for (size_t i = 0; i < NELEMS(log_fds); ++i) + if (log_fds[i] >= 0) + close(log_fds[i]); +} + /** * @brief Initialize the backend * @details Prepares the backend for proper and fruitful work @@ -91,6 +101,7 @@ void __dlog_init_android(const struct log_config *conf) continue; } write_to_log = __write_to_log_android; + destroy_backend = __destroy_androidlogger; return; failure: diff --git a/src/libdlog/log_pipe.c b/src/libdlog/log_pipe.c index 84b1e2b..8367bfc 100644 --- a/src/libdlog/log_pipe.c +++ b/src/libdlog/log_pipe.c @@ -179,6 +179,19 @@ static int __write_to_log_pipe(log_id_t log_id, log_priority prio, const char *t return ret; } +static void __destroy_pipe() +{ + /* No need for an explicit lock here; this should only be called + * by the library destructor which has its own lock */ + + for (size_t i = 0; i < NELEMS(pipe_fd); ++i) + if (pipe_fd[i] >= 0) + close(pipe_fd[i]); + + wait_pipe_ms = DEFAULT_WAIT_PIPE_MS; + signal(SIGPIPE, SIG_DFL); +} + /** * @brief Initialize the backend * @details Prepares the backend for proper and fruitful work @@ -218,4 +231,5 @@ void __dlog_init_pipe(const struct log_config *conf) snprintf(log_pipe_path[i], PATH_MAX, "%s", conf_val); } write_to_log = __write_to_log_pipe; + destroy_backend = __destroy_pipe; } diff --git a/src/tests/libdlog_android.c b/src/tests/libdlog_android.c index ca699e8..5bd3b52 100644 --- a/src/tests/libdlog_android.c +++ b/src/tests/libdlog_android.c @@ -7,6 +7,7 @@ #include int (*write_to_log)(log_id_t, log_priority, const char *tag, const char *msg) = NULL; +void (*destroy_backend)() = NULL; extern void __dlog_init_android(const struct log_config *conf); static struct log_config CONF; @@ -89,12 +90,14 @@ int main() __dlog_init_android(&CONF); assert(closed == 2); assert(!write_to_log); + assert(!destroy_backend); closed = 0; fail_open = false; __dlog_init_android(&CONF); assert(closed == 0); assert(write_to_log); + assert(destroy_backend); #define F(B, P, M) assert(write_to_log(B, P, "tag", M) == DLOG_ERROR_INVALID_PARAMETER) F(LOG_ID_INVALID, DLOG_ERROR , "xx"); @@ -125,4 +128,7 @@ int main() used_msg = LONG_TEXTS[0]; writev_ret = 1234; assert(write_to_log(0, 3, NULL, LONG_TEXTS[0]) == 1234); + + destroy_backend(); + assert(closed + 1 == LOG_ID_MAX); } diff --git a/src/tests/libdlog_pipe.c b/src/tests/libdlog_pipe.c index 3d38182..cb814fa 100644 --- a/src/tests/libdlog_pipe.c +++ b/src/tests/libdlog_pipe.c @@ -11,7 +11,8 @@ #include int (*write_to_log)(log_id_t, log_priority, const char *tag, const char *msg) = NULL; -pthread_rwlock_t log_limiter_lock = PTHREAD_RWLOCK_INITIALIZER; +void (*destroy_backend)() = NULL; + extern void __dlog_init_pipe(const struct log_config *conf); int errno_to_set; @@ -79,6 +80,13 @@ int __wrap_socket(int domain, int type, int protocol) return 17; } +static size_t closed; +int __wrap_close(int fd) +{ + assert(fd == 1337 || fd == 17); + ++closed; +} + int main() { struct log_config conf = {NULL, NULL}; @@ -104,6 +112,7 @@ int main() __dlog_init_pipe(&conf); assert(critical_failure_detected); assert(!write_to_log); + assert(!destroy_backend); critical_failure_detected = false; pipe_path[0] = '/'; @@ -112,6 +121,7 @@ int main() __dlog_init_pipe(&conf); assert(critical_failure_detected); assert(!write_to_log); + assert(!destroy_backend); critical_failure_detected = false; pipe_path[4] = '\0'; @@ -119,6 +129,7 @@ int main() __dlog_init_pipe(&conf); assert(!critical_failure_detected); assert(write_to_log); + assert(destroy_backend); #define ACTUAL_PAYLOAD_SIZE (LOG_MAX_PAYLOAD_SIZE - 5) // 1 priority, 3 tag "xx\0", 1 null terminator char payload[ACTUAL_PAYLOAD_SIZE + 1] = { [0 ... ACTUAL_PAYLOAD_SIZE] = 'x' }; @@ -177,4 +188,8 @@ int main() // NULL tag is fine, apparently. TC(0, 0, 0, 45, 301, NULL, 0, 1, 0); #undef TC + + closed = 0; + destroy_backend(); + assert(closed == 1); // just MAIN, we'd never used the others } -- 2.7.4