[PATCH v4 0/3] xl/libxl: fix some issues discovered by coverity

[PATCH v4 0/3] xl/libxl: fix some issues discovered by coverity

[Wei Liu]

Wei Liu (3):
  libxl: make libxl__strdup and libxl__strndup handle NULL
  libxl: localtime(3) can return NULL
  libxl: call libxl_cpupoolinfo_{init,dispose} in numa_place_domain

 tools/libxl/libxl_dom.c      |  5 +++--
 tools/libxl/libxl_internal.c | 12 ++++++++++--
 tools/libxl/libxl_internal.h |  7 +++++--
 tools/libxl/libxl_x86.c      |  7 +++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH v4 1/3] libxl: make libxl__strdup and libxl__strndup handle NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v4: add doc, also handle strndup variant.
---
 tools/libxl/libxl_internal.c | 12 ++++++++++--
 tools/libxl/libxl_internal.h |  7 +++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 24a0901..23fd751 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -161,7 +161,11 @@ char *libxl__sprintf(libxl__gc *gc, const char *fmt, ...)
 
 char *libxl__strdup(libxl__gc *gc, const char *c)
 {
-    char *s = strdup(c);
+    char *s;
+
+    if (!c) return NULL;
+
+    s = strdup(c);
 
     if (!s) libxl__alloc_failed(CTX, __func__, strlen(c), 1);
 
@@ -172,7 +176,11 @@ char *libxl__strdup(libxl__gc *gc, const char *c)
 
 char *libxl__strndup(libxl__gc *gc, const char *c, size_t n)
 {
-    char *s = strndup(c, n);
+    char *s;
+
+    if (!c) return NULL;
+
+    s = strndup(c, n);
 
     if (!s) libxl__alloc_failed(CTX, __func__, n, 1);
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2b6b2a0..76114d3 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -608,9 +608,12 @@ _hidden void *libxl__realloc(libxl__gc *gc_opt, void *ptr, size_t new_size) NN1;
 _hidden char *libxl__sprintf(libxl__gc *gc_opt, const char *fmt, ...) PRINTF_ATTRIBUTE(2, 3) NN1;
 _hidden char *libxl__vsprintf(libxl__gc *gc, const char *format, va_list ap);
 /* duplicate the string @c (similar to a gc'd strdup(3)). */
-_hidden char *libxl__strdup(libxl__gc *gc_opt, const char *c) NN1;
+_hidden char *libxl__strdup(libxl__gc *gc_opt,
+                            const char *c /* may be NULL */) NN1;
 /* duplicate at most @n bytes of string @c (similar to a gc'd strndup(3)). */
-_hidden char *libxl__strndup(libxl__gc *gc_opt, const char *c, size_t n) NN1;
+_hidden char *libxl__strndup(libxl__gc *gc_opt,
+                             const char *c /* may be NULL */,
+                             size_t n) NN1;
 /* strip the last path component from @s and return as a newly allocated
  * string. (similar to a gc'd dirname(3)). */
 _hidden char *libxl__dirname(libxl__gc *gc_opt, const char *s) NN1;
-- 
1.9.1

Re: [PATCH v4 1/3] libxl: make libxl__strdup and libxl__strndup handle NULL

[Ian Jackson]

Wei Liu writes ("[PATCH v4 1/3] libxl: make libxl__strdup and libxl__strndup handle NULL"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v4 1/3] libxl: make libxl__strdup and libxl__strndup handle NULL

[Ian Campbell]

On Fri, 2015-07-17 at 18:03 +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v4 1/3] libxl: make libxl__strdup and 
> libxl__strndup handle NULL"):
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied. thanks.

Ian.

[PATCH v4 2/3] libxl: localtime(3) can return NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v4: add logging
---
 tools/libxl/libxl_x86.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 8cd15ca..ce14a73 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -311,6 +311,12 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
         t = time(NULL);
         tm = localtime(&t);
 
+        if (!tm) {
+            LOG(ERROR, "Failed to call localtime");
+            ret = ERROR_FAIL;
+            goto out;
+        }
+
         rtc_timeoffset += tm->tm_gmtoff;
     }
 
@@ -335,6 +341,7 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
         }
     }
 
+out:
     return ret;
 }
 
-- 
1.9.1

Re: [PATCH v4 2/3] libxl: localtime(3) can return NULL

[Ian Jackson]

Wei Liu writes ("[PATCH v4 2/3] libxl: localtime(3) can return NULL"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

>          tm = localtime(&t);
> +        if (!tm) {
> +            LOG(ERROR, "Failed to call localtime");

localtime sets errno if it fails.  So you mean LOGE.

Ian.

Re: [PATCH v4 2/3] libxl: localtime(3) can return NULL

[Wei Liu]

On Fri, Jul 17, 2015 at 06:05:45PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v4 2/3] libxl: localtime(3) can return NULL"):
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> >          tm = localtime(&t);
> > +        if (!tm) {
> > +            LOG(ERROR, "Failed to call localtime");
> 
> localtime sets errno if it fails.  So you mean LOGE.
> 

Heh. Linux manpage doesn't say so. But

http://pubs.opengroup.org/onlinepubs/009695399/functions/localtime.html

does say that.

I will rework this patch and send it out next week with other coverity
scan inspired patches I accumulate.

> Ian.

Re: [PATCH v4 2/3] libxl: localtime(3) can return NULL

[Ian Campbell]

On Fri, 2015-07-17 at 18:12 +0100, Wei Liu wrote:
> On Fri, Jul 17, 2015 at 06:05:45PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("[PATCH v4 2/3] libxl: localtime(3) can return 
> > NULL"):
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > >          tm = localtime(&t);
> > > +        if (!tm) {
> > > +            LOG(ERROR, "Failed to call localtime");
> > 
> > localtime sets errno if it fails.  So you mean LOGE.
> > 
> 
> Heh. Linux manpage doesn't say so. But
> 
> http://pubs.opengroup.org/onlinepubs/009695399/functions/localtime.ht
> ml
> 
> does say that.
> 
> I will rework this patch and send it out next week with other 
> coverity
> scan inspired patches I accumulate.

Should we not be using localtime_t in libxl? Afterall we don't know
what other threads in the application might be doing.

Re: [PATCH v4 2/3] libxl: localtime(3) can return NULL

[Wei Liu]

On Wed, Jul 22, 2015 at 03:19:08PM +0100, Ian Campbell wrote:
> On Fri, 2015-07-17 at 18:12 +0100, Wei Liu wrote:
> > On Fri, Jul 17, 2015 at 06:05:45PM +0100, Ian Jackson wrote:
> > > Wei Liu writes ("[PATCH v4 2/3] libxl: localtime(3) can return 
> > > NULL"):
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > 
> > > >          tm = localtime(&t);
> > > > +        if (!tm) {
> > > > +            LOG(ERROR, "Failed to call localtime");
> > > 
> > > localtime sets errno if it fails.  So you mean LOGE.
> > > 
> > 
> > Heh. Linux manpage doesn't say so. But
> > 
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/localtime.ht
> > ml
> > 
> > does say that.
> > 
> > I will rework this patch and send it out next week with other 
> > coverity
> > scan inspired patches I accumulate.
> 
> Should we not be using localtime_t in libxl? Afterall we don't know
> what other threads in the application might be doing.
> 

I guess you mean localtime_r.

I agree with you we should use that one.

Wei.

Re: [PATCH v4 2/3] libxl: localtime(3) can return NULL

[Ian Campbell]

On Wed, 2015-07-22 at 16:09 +0100, Wei Liu wrote:
> On Wed, Jul 22, 2015 at 03:19:08PM +0100, Ian Campbell wrote:
> > On Fri, 2015-07-17 at 18:12 +0100, Wei Liu wrote:
> > > On Fri, Jul 17, 2015 at 06:05:45PM +0100, Ian Jackson wrote:
> > > > Wei Liu writes ("[PATCH v4 2/3] libxl: localtime(3) can return 
> > > > NULL"):
> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > 
> > > > >          tm = localtime(&t);
> > > > > +        if (!tm) {
> > > > > +            LOG(ERROR, "Failed to call localtime");
> > > > 
> > > > localtime sets errno if it fails.  So you mean LOGE.
> > > > 
> > > 
> > > Heh. Linux manpage doesn't say so. But
> > > 
> > > http://pubs.opengroup.org/onlinepubs/009695399/functions/localtim
> > > e.ht
> > > ml
> > > 
> > > does say that.
> > > 
> > > I will rework this patch and send it out next week with other 
> > > coverity
> > > scan inspired patches I accumulate.
> > 
> > Should we not be using localtime_t in libxl? Afterall we don't know
> > what other threads in the application might be doing.
> > 
> 
> I guess you mean localtime_r.

yes.

> I agree with you we should use that one.
> 
> Wei.

[PATCH v4 3/3] libxl: call libxl_cpupoolinfo_{init, dispose} in numa_place_domain

[Wei Liu]

Call libxl_cpupoolinfo_init at the beginning.  Change two returns to
goto out so that libxl_cpupoolinfo_dispose is called in failure path.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dom.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 4cb247a..35eadc2 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -142,6 +142,7 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
 
     libxl__numa_candidate_init(&candidate);
     libxl_bitmap_init(&cpupool_nodemap);
+    libxl_cpupoolinfo_init(&cpupool_info);
 
     /*
      * Extract the cpumap from the cpupool the domain belong to. In fact,
@@ -150,10 +151,10 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
      */
     rc = cpupool = libxl__domain_cpupool(gc, domid);
     if (rc < 0)
-        return rc;
+        goto out;
     rc = libxl_cpupool_info(CTX, &cpupool_info, cpupool);
     if (rc)
-        return rc;
+        goto out;
 
     rc = libxl_domain_need_memory(CTX, info, &memkb);
     if (rc)
-- 
1.9.1

Re: [PATCH v4 3/3] libxl: call libxl_cpupoolinfo_{init, dispose} in numa_place_domain

[Ian Jackson]

Wei Liu writes ("[PATCH v4 3/3] libxl: call libxl_cpupoolinfo_{init,dispose} in numa_place_domain"):
> Call libxl_cpupoolinfo_init at the beginning.  Change two returns to
> goto out so that libxl_cpupoolinfo_dispose is called in failure path.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v4 3/3] libxl: call libxl_cpupoolinfo_{init, dispose} in numa_place_domain

[Ian Campbell]

On Fri, 2015-07-17 at 18:03 +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v4 3/3] libxl: call 
> libxl_cpupoolinfo_{init,dispose} in numa_place_domain"):
> > Call libxl_cpupoolinfo_init at the beginning.  Change two returns 
> > to
> > goto out so that libxl_cpupoolinfo_dispose is called in failure 
> > path.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied.