Skip to content

Commit

Permalink
logsource: add explicit (un)initialized state to WindowSizeCounter
Browse files Browse the repository at this point in the history
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 <laszlo.budai@outlook.com>
  • Loading branch information
lbudai committed Aug 29, 2019
1 parent 0262678 commit e350607
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 6 deletions.
23 changes: 17 additions & 6 deletions lib/logsource.c
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down
1 change: 1 addition & 0 deletions lib/logsource.h
Expand Up @@ -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;
Expand Down

0 comments on commit e350607

Please sign in to comment.