[libvirt] [PATCH 2/2] remote: fix crash on OOM

[Daniel Veillard]

On Tue, Sep 20, 2011 at 12:11:32PM -0600, Eric Blake wrote:
> Bug introduced in commit 675464b.  On an OOM, this would try to
> dereference a char* and free the contents as a pointer, which is
> doomed to failure.
> 
> Adding a syntax check will prevent mistakes like this in the future.
> 
> * cfg.mk (sc_prohibit_internal_functions): New syntax check.
> (exclude_file_name_regexp--sc_prohibit_internal_functions): Add
> exemptions.
> * daemon/remote.c (remoteRelayDomainEventIOError)
> (remoteRelayDomainEventIOErrorReason)
> (remoteRelayDomainEventGraphics, remoteRelayDomainEventBlockJob):
> Use correct free function.
> ---
>  cfg.mk          |   11 ++++++++++-
>  daemon/remote.c |   28 ++++++++++++++--------------
>  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 95c5eff..9f4aa3e 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -212,7 +212,7 @@ useless_free_options =				\
>  # y virDomainWatchdogDefFree
>  # n virDrvNodeGetCellsFreeMemory (returns int)
>  # n virDrvNodeGetFreeMemory (returns long long)
> -# n virFree (dereferences param)
> +# n virFree - dereferences param
>  # n virFreeError
>  # n virHashFree (takes 2 args)
>  # y virInterfaceDefFree
> @@ -306,6 +306,12 @@ sc_flags_usage:
>  	halt='flags should be unsigned'					\
>  	  $(_sc_search_regexp)
> 
> +# Avoid functions that should only be called via macro counterparts.
> +sc_prohibit_internal_functions:
> +	@prohibit='vir(Free|AllocN?|ReallocN|File(Close|Fclose|Fdopen)) *\(' \
> +	halt='use VIR_ macros instead of internal functions'		\
> +	  $(_sc_search_regexp)
> +
>  # Avoid functions that can lead to double-close bugs.
>  sc_prohibit_close:
>  	@prohibit='([^>.]|^)\<[fp]?close *\('				\
> @@ -706,6 +712,9 @@ exclude_file_name_regexp--sc_prohibit_fork_wrappers = \
> 
>  exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/util\.c$$
> 
> +exclude_file_name_regexp--sc_prohibit_internal_functions = \
> +  ^src/(util/(memory|util|virfile)\.[hc]|esx/esx_vi\.c)$$
> +
>  exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \
>    ^src/rpc/gendispatch\.pl$$
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 74e759a..245d41c 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -250,8 +250,8 @@ static int remoteRelayDomainEventIOError(virConnectPtr conn ATTRIBUTE_UNUSED,
>      return 0;
>  mem_error:
>      virReportOOMError();
> -    virFree(data.srcPath);
> -    virFree(data.devAlias);
> +    VIR_FREE(data.srcPath);
> +    VIR_FREE(data.devAlias);
>      return -1;
>  }
> 
> @@ -296,9 +296,9 @@ static int remoteRelayDomainEventIOErrorReason(virConnectPtr conn ATTRIBUTE_UNUS
> 
>  mem_error:
>      virReportOOMError();
> -    virFree(data.srcPath);
> -    virFree(data.devAlias);
> -    virFree(data.reason);
> +    VIR_FREE(data.srcPath);
> +    VIR_FREE(data.devAlias);
> +    VIR_FREE(data.reason);
>      return -1;
>  }
> 
> @@ -374,17 +374,17 @@ static int remoteRelayDomainEventGraphics(virConnectPtr conn ATTRIBUTE_UNUSED,
> 
>  mem_error:
>      virReportOOMError();
> -    virFree(data.authScheme);
> -    virFree(data.local.node);
> -    virFree(data.local.service);
> -    virFree(data.remote.node);
> -    virFree(data.remote.service);
> +    VIR_FREE(data.authScheme);
> +    VIR_FREE(data.local.node);
> +    VIR_FREE(data.local.service);
> +    VIR_FREE(data.remote.node);
> +    VIR_FREE(data.remote.service);
>      if (data.subject.subject_val != NULL) {
>          for (i = 0 ; i < data.subject.subject_len ; i++) {
> -            virFree(data.subject.subject_val[i].type);
> -            virFree(data.subject.subject_val[i].name);
> +            VIR_FREE(data.subject.subject_val[i].type);
> +            VIR_FREE(data.subject.subject_val[i].name);
>          }
> -        virFree(data.subject.subject_val);
> +        VIR_FREE(data.subject.subject_val);
>      }
>      return -1;
>  }
> @@ -422,7 +422,7 @@ static int remoteRelayDomainEventBlockJob(virConnectPtr conn ATTRIBUTE_UNUSED,
> 
>  mem_error:
>      virReportOOMError();
> -    virFree(data.path);
> +    VIR_FREE(data.path);
>      return -1;
>  }
> 

  Gahhh, I got it wrong, I wanted to use VIR_FREE and failed to notice
my mistake ...
  ACK,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/