* [PATCH] libxl: Fix error handling in libxl_userdata_unlink
[not found] <5422aad1e5ef3_72bf92732c895b9@scan.coverity.com.mail>
@ 2014-09-24 14:30 ` Ian Jackson
2014-09-24 14:39 ` Andrew Cooper
2014-10-02 8:59 ` Wei Liu
0 siblings, 2 replies; 4+ messages in thread
From: Ian Jackson @ 2014-09-24 14:30 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Ian Jackson, security
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 <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;
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: Fix error handling in libxl_userdata_unlink
2014-09-24 14:30 ` [PATCH] libxl: Fix error handling in libxl_userdata_unlink Ian Jackson
@ 2014-09-24 14:39 ` Andrew Cooper
2014-10-08 10:56 ` Ian Jackson
2014-10-02 8:59 ` Wei Liu
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2014-09-24 14:39 UTC (permalink / raw)
To: Ian Jackson, xen-devel; +Cc: Wei Liu, security
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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: Fix error handling in libxl_userdata_unlink
2014-09-24 14:30 ` [PATCH] libxl: Fix error handling in libxl_userdata_unlink Ian Jackson
2014-09-24 14:39 ` Andrew Cooper
@ 2014-10-02 8:59 ` Wei Liu
1 sibling, 0 replies; 4+ messages in thread
From: Wei Liu @ 2014-10-02 8:59 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Wei Liu, security, Ian Campbell
Ian C, this one seems to have fell through the crack.
Ian J CC'ed himself twice in the original email. :-)
Wei.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: Fix error handling in libxl_userdata_unlink
2014-09-24 14:39 ` Andrew Cooper
@ 2014-10-08 10:56 ` Ian Jackson
0 siblings, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2014-10-08 10:56 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, coverity
Andrew Cooper writes ("Re: [Xen-devel] [PATCH] libxl: Fix error handling in libxl_userdata_unlink"):
> On 24/09/14 15:30, Ian Jackson wrote:
> > (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@ ?
Yes.
> > - libxl__domain_userdata_lock *lock;
> > + int rc;
> > + libxl__domain_userdata_lock *lock = 0;
>
> Pointers should be initialised to NULL rather than 0.
Changed.
> With this change, Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
Now pushed (with Ian C's ack, also).
Ian.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-08 10:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <5422aad1e5ef3_72bf92732c895b9@scan.coverity.com.mail>
2014-09-24 14:30 ` [PATCH] libxl: Fix error handling in libxl_userdata_unlink Ian Jackson
2014-09-24 14:39 ` Andrew Cooper
2014-10-08 10:56 ` Ian Jackson
2014-10-02 8:59 ` Wei Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.