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

[Daniel P. Berrangé]

On Wed, Aug 19, 2020 at 12:03:40PM +0200, Natanael Copa wrote:
> Doing malloc/free after fork is techincally not allowed in POSIX and
> deadlocks[1] with musl libc.

Removing one free() call isn't going to make the logging code
work with musl. It merely hides one isolated instance that
happens to have been hit in the reset code. The log setup
code will malloc, as well actually emitting a log message which
triggers a allocation during printf formatting of the message.

> 
> [1]: https://gitlab.com/libvirt/libvirt/-/issues/52
> 
> Signed-off-by: Natanael Copa <ncopa at alpinelinux.org>
> ---
>  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
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|