Re: [PATCH 1/2] util: avoid free() when reset log after fork

Doing malloc/free after fork is techincally not allowed in POSIX and deadlocks[1] with musl libc.

[1]: https://gitlab.com/libvirt/libvirt/-/issues/52 Signed-off-by: Natanael Copa <ncopa(a)alpinelinux.org&gt; --- src/util/vircommand.c | 4 ++-- src/util/virlog.c | 44 +++++++++++++++++++++++++++++++++---------- src/util/virlog.h | 1 + 3 files changed, 37 insertions(+), 12 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 76f7eb9a3d..17e5bb00d3 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -304,7 +304,7 @@ virFork(void) /* Make sure any hook logging is sent to stderr, since child * process may close the logfile FDs */ logprio = virLogGetDefaultPriority(); - virLogReset(); + virLogResetWithoutFree(); virLogSetDefaultPriority(logprio); /* Clear out all signal handlers from parent so nothing @@ -861,7 +861,7 @@ virExec(virCommandPtr cmd) goto fork_error; /* Close logging again to ensure no FDs leak to child */ - virLogReset(); + virLogResetWithoutFree(); if (cmd->env) execve(binary, cmd->args, cmd->env); diff --git a/src/util/virlog.c b/src/util/virlog.c index 3217e5eb73..3959de5ca7 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -108,8 +108,8 @@ static size_t virLogNbOutputs; */ static virLogPriority virLogDefaultPriority = VIR_LOG_DEFAULT; -static void virLogResetFilters(void); -static void virLogResetOutputs(void); +static void virLogResetFilters(bool freemem); +static void virLogResetOutputs(bool freemem); static void virLogOutputToFd(virLogSourcePtr src, virLogPriority priority, const char *filename, @@ -284,8 +284,30 @@ virLogReset(void) return -1; virLogLock(); - virLogResetFilters(); - virLogResetOutputs(); + virLogResetFilters(true); + virLogResetOutputs(true); + virLogDefaultPriority = VIR_LOG_DEFAULT; + virLogUnlock(); + return 0; +} + +/** + * virLogResetWithoutFree: + * + * Reset the logging module to its default initial state, but avoid doing + * free() so it can be used after fork and before exec. + * + * Returns 0 if successful, and -1 in case or error + */ +int +virLogResetWithoutFree(void) +{ + if (virLogInitialize() < 0) + return -1; + + virLogLock(); + virLogResetFilters(false); + virLogResetOutputs(false); virLogDefaultPriority = VIR_LOG_DEFAULT; virLogUnlock(); return 0; @@ -324,9 +346,10 @@ virLogSetDefaultPriority(virLogPriority priority) * Removes the set of logging filters defined. */ static void -virLogResetFilters(void) +virLogResetFilters(bool freemem) { - virLogFilterListFree(virLogFilters, virLogNbFilters); + if (freemem) + virLogFilterListFree(virLogFilters, virLogNbFilters); virLogFilters = NULL; virLogNbFilters = 0; virLogFiltersSerial++; @@ -371,9 +394,10 @@ virLogFilterListFree(virLogFilterPtr *list, int count) * Removes the set of logging output defined. */ static void -virLogResetOutputs(void) +virLogResetOutputs(bool freemem) { - virLogOutputListFree(virLogOutputs, virLogNbOutputs); + if (freemem) + virLogOutputListFree(virLogOutputs, virLogNbOutputs); virLogOutputs = NULL; virLogNbOutputs = 0; } @@ -1392,7 +1416,7 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs) return -1; virLogLock(); - virLogResetOutputs(); + virLogResetOutputs(true); #if HAVE_SYSLOG_H /* syslog needs to be special-cased, since it keeps the fd in private */ @@ -1435,7 +1459,7 @@ virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters) return -1; virLogLock(); - virLogResetFilters(); + virLogResetFilters(true); virLogFilters = filters; virLogNbFilters = nfilters; virLogUnlock(); diff --git a/src/util/virlog.h b/src/util/virlog.h index 984a9d5a43..69f7b1ef94 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -168,6 +168,7 @@ void virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged); void virLogLock(void); void virLogUnlock(void); int virLogReset(void); +int virLogResetWithoutFree(void); int virLogParseDefaultPriority(const char *priority); int virLogPriorityFromSyslog(int priority); void virLogMessage(virLogSourcePtr source, -- 2.28.0