From 32f0cd32429fa79445a1f9d0e4de1aaf63701102 Mon Sep 17 00:00:00 2001 From: Aurelien DARRAGON Date: Tue, 14 May 2024 10:22:19 +0200 Subject: [PATCH] BUG/MINOR: log: smp_rgs array issues with inherited global log directives When a log directive is defined in the global section, each time we use "log global" in a proxy section, the global log directives are duplicated for the current proxy. This works by creating a new proxy logger struct and duplicating every members for each global one. However, smp_rgs logger member is a special pointer member that is allocated when "range" is used on a log directive. Currently, we simply copy the array pointer (from the global one), instead of creating our own copy. Because of that, range log sampling may not work properly in some situations prior to 3f1284560 ("MINOR: log: remove the unused curr_idx in struct smp_log_range") when used in global log directives, for instance: global log 127.0.0.1:5114 format raw sample 1-2,3:4 local0 info # should receive 75% of all proxy logs log 127.0.0.1:5115 format raw sample 4:4 local0 info # should receive 25% of all proxy logs listen proxy1 log global listen proxy2 log global May not work as expected, because curr_idx was stored within smp_rgs array member prior to 3f1284560, and due to this bug, it happens to be shared between every log directive inherited from a "global" one. The result is that curr_idx counter will not behave properly because the index will be increased globally instead of per-log directive, and it could even suffer from concurrent thread accesses under load since we don't own the global log directive's lock when manipulating it. Another issue that was revealed because of this bug is that the smp_rgs array allocated during config parsing is never freed in free_logger(), resulting in small memory leak during clean exit. To fix these issues all at once, let's properly duplicate smp_rgs logger struct member in dup_logger() like we already do for other special members so that every log directive have its own sms_rgs copy, and then systematically free it in free_logger(). While this bug affects all stable versions (including 2.4), it's probably best to not backport this beyond 2.6 because of 211ea252d ("BUG/MINOR: logs: fix logsrv leaks on clean exit") prerequisite that first appears in 2.6. [ada: for versions prior to 2.9, 969e212 ("MINOR: log: add dup_logsrv() helper function") and 76acde91 ("BUG/MINOR: log: keep the ref in dup_logger()") must be backported first. Note: Some ctx adjustments should be performed because 'logger' struct used to be named 'logsrv' in the past and 2.9 introduced logger target struct member. Thus it's probably easier to manually apply 76acde91 and the current bugfix by hand directly on top of 969e212. ] --- src/log.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/log.c b/src/log.c index 5ae0ec0ea..3d66e7a8c 100644 --- a/src/log.c +++ b/src/log.c @@ -1423,6 +1423,7 @@ struct logger *dup_logger(struct logger *def) /* default values */ cpy->conf.file = NULL; + cpy->lb.smp_rgs = NULL; LIST_INIT(&cpy->list); /* special members */ @@ -1433,6 +1434,13 @@ struct logger *dup_logger(struct logger *def) if (!cpy->conf.file) goto error; } + if (def->lb.smp_rgs) { + cpy->lb.smp_rgs = malloc(sizeof(*cpy->lb.smp_rgs) * def->lb.smp_rgs_sz); + if (!cpy->lb.smp_rgs) + goto error; + memcpy(cpy->lb.smp_rgs, def->lb.smp_rgs, + sizeof(*cpy->lb.smp_rgs) * def->lb.smp_rgs_sz); + } /* inherit from original reference if set */ cpy->ref = (def->ref) ? def->ref : def; @@ -1456,6 +1464,7 @@ void free_logger(struct logger *logger) BUG_ON(LIST_INLIST(&logger->list)); ha_free(&logger->conf.file); deinit_log_target(&logger->target); + free(logger->lb.smp_rgs); free(logger); }