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

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; }