From e350607f27b78094fb72422faf5394384ae6193b Mon Sep 17 00:00:00 2001 From: Laszlo Budai Date: Thu, 29 Aug 2019 17:09:39 +0200 Subject: [PATCH] logsource: add explicit (un)initialized state to WindowSizeCounter Fixes: #2893 On 32 bit systems (or non-64 bit systems), syslog-ng could abort during shutdown. What was the reason of the abort? a) in `log_source_set_options` where we set the initial window size conditionally, the condition was false thus the `full_window_size` remained 0 b) when `log_source_free` is called during shutdown, * `_release_dynamic_window` called unconditionally and * a dynamic_part is calculated as full_window_size(=0) - init_window_size(=default 100), so dynamic_part = -100 * window_size is decremented by dynamic_part(-100) and the `window_size_counter_sub` asserts on old_value >= value, and this assert failed, so syslog-ng aborted So the questions are 1) why we did not set initial window size? 2) why old_value was not greater than value? Answers: 1) the value we get from `window_size_counter_get` is the masked value... on 64 bit systems this value is a 63 bits of `1` and it is compared to a 32 bits of `1` but the 63 bits are truncated to 32 thanks to an explicit cast And what if we are on a 32 bits system? Well... the sizeof(gsize) is 4 , sizeof(gint) is also 4 on these systems. This means that the `window_size_counter_get` returns 31 bits of `-1`, and it is compared to 32 bits of `1` : they are obviously not equals -> we won't set full_window_size 2) old_value is a -1, which is masked, so the actual old value is 2^31-1, while new value is a -100, which is (2^32-100), so on a 32 bits system 31 bit negative value is compared to a to 32 bits negative value... Proposed solution: * add a initialized state to LogSource: this is checked/(set to TRUE) only in `log_source_set_options`, and set to FALSE only in `log_source_init_instance` Signed-off-by: Laszlo Budai [Retrieved from: https://github.com/syslog-ng/syslog-ng/commit/e350607f27b78094fb72422faf5394384ae6193b] Signed-off-by: Ricardo Martincoski --- lib/logsource.c | 23 +++++++++++++++++------ lib/logsource.h | 1 + 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/logsource.c b/lib/logsource.c index 3f38b66e8..67e1c1570 100644 --- a/lib/logsource.c +++ b/lib/logsource.c @@ -633,7 +633,20 @@ log_source_queue(LogPipe *s, LogMessage *msg, const LogPathOptions *path_options evt_tag_printf("msg", "%p", msg)); msg_set_context(NULL); +} + +static void +_initialize_window(LogSource *self, gint init_window_size) +{ + self->window_initialized = TRUE; + window_size_counter_set(&self->window_size, init_window_size); + self->full_window_size = init_window_size; +} +static gboolean +_is_window_initialized(LogSource *self) +{ + return self->window_initialized; } void @@ -645,11 +658,9 @@ log_source_set_options(LogSource *self, LogSourceOptions *options, * configuration and we received a SIGHUP. This means that opened * connections will not have their window_size changed. */ - if ((gint)window_size_counter_get(&self->window_size, NULL) == -1) - { - window_size_counter_set(&self->window_size, options->init_window_size); - self->full_window_size = options->init_window_size; - } + if (!_is_window_initialized(self)) + _initialize_window(self, options->init_window_size); + self->options = options; if (self->stats_id) g_free(self->stats_id); @@ -679,7 +690,7 @@ log_source_init_instance(LogSource *self, GlobalConfig *cfg) self->super.free_fn = log_source_free; self->super.init = log_source_init; self->super.deinit = log_source_deinit; - window_size_counter_set(&self->window_size, (gsize)-1); + self->window_initialized = FALSE; self->ack_tracker = NULL; } diff --git a/lib/logsource.h b/lib/logsource.h index 370842efc..75d492604 100644 --- a/lib/logsource.h +++ b/lib/logsource.h @@ -71,6 +71,7 @@ struct _LogSource gchar *stats_instance; WindowSizeCounter window_size; DynamicWindow dynamic_window; + gboolean window_initialized; /* full_window_size = static + dynamic */ gsize full_window_size; atomic_gssize window_size_to_be_reclaimed; -- 2.17.1