From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] libxl: Fix error handling in
	libxl_userdata_unlink
Date: Wed, 24 Sep 2014 15:39:34 +0100
Message-ID: <5422D7A6.5030201@citrix.com>
References: <5422aad1e5ef3_72bf92732c895b9@scan.coverity.com.mail>
	<1411569004-30623-1-git-send-email-ian.jackson@eu.citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1411569004-30623-1-git-send-email-ian.jackson@eu.citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Jackson <ian.jackson@eu.citrix.com>, xen-devel@lists.xensource.com
Cc: Wei Liu <wei.liu2@citrix.com>, security@xenproject.org
List-Id: xen-devel@lists.xenproject.org

On 24/09/14 15:30, Ian Jackson wrote:
> 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.)

How about coverty@ which includes some of us not on securty@ ?

>
> Coverity-ID: 1240237, 1240235.
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: security@xenproject.org
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  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;

Pointers should be initialised to NULL rather than 0.

With this change, Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>

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