From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: [PATCH] libxl: Fix error handling in libxl_userdata_unlink Date: Wed, 24 Sep 2014 15:30:04 +0100 Message-ID: <1411569004-30623-1-git-send-email-ian.jackson@eu.citrix.com> References: <5422aad1e5ef3_72bf92732c895b9@scan.coverity.com.mail> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5422aad1e5ef3_72bf92732c895b9@scan.coverity.com.mail> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xensource.com Cc: Wei Liu , Ian Jackson , security@xenproject.org List-Id: xen-devel@lists.xenproject.org Previously: * rc would not be set before leaving the function, with the result that an uninitialised value would be returned * failures of libxl__userdata_path would result in a NULL dereference * failures of unlink() would not be usefully logged This appears to be due to an attempt to avoid having to repeat the call to libxl__unlock_domain_userdata by informally sharing parts of the success and failure paths. Change to use the canonical error-handling style: * Initialise lock to 0. * Do the unlock in the `out' section - always attempt to unlock lock if it is non-0. * Explicitly set rc and `goto out' on all error paths, even those right at the end of the function. * Add an error check for filename = libxl__userdata_path(...); (CCing security@ because they receive the Coverity reports. This is not a security problem AFAICT.) Coverity-ID: 1240237, 1240235. CC: Wei Liu CC: security@xenproject.org Signed-off-by: Ian Jackson --- tools/libxl/libxl_dom.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index bd21841..9eb74ec 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -2097,12 +2097,12 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid, const char *userdata_userid) { GC_INIT(ctx); - int rc; + CTX_LOCK; - libxl__domain_userdata_lock *lock; + int rc; + libxl__domain_userdata_lock *lock = 0; const char *filename; - CTX_LOCK; lock = libxl__lock_domain_userdata(gc, domid); if (!lock) { rc = ERROR_LOCK_FAIL; @@ -2110,10 +2110,20 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid, } filename = libxl__userdata_path(gc, domid, userdata_userid, "d"); - if (unlink(filename)) rc = ERROR_FAIL; + if (!filename) { + rc = ERROR_FAIL; + goto out; + } + if (unlink(filename)) { + LOGE(ERROR, "error deleting userdata file: %s", filename); + rc = ERROR_FAIL; + goto out; + } - libxl__unlock_domain_userdata(lock); + rc = 0; out: + if (lock) + libxl__unlock_domain_userdata(lock); CTX_UNLOCK; GC_FREE; return rc; -- 1.7.10.4