Coverity reports that there is a memory leak in
ioreq_server_alloc_rangesets().  This would be true if Xen's implementation of
asprintf() had glibc's return semantics, but it doesn't.

Rename to xasprintf() to reduce confusion for Coverity and other developers.

While at it, fix style issues.  Rearrange ioreq_server_alloc_rangesets() to
use a tabulated switch statement, and not to have a trailing space in the
rangeset name for an unknown range type.

Coverity-ID: 1472735
Coverity-ID: 1500265
Fixes: 780e918a2e54 ("add an implentation of asprintf() for xen")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Paul Durrant <paul@xen.org>
---
 xen/common/ioreq.c    | 16 ++++++++++------
 xen/common/vsprintf.c | 11 ++++++-----
 xen/include/xen/lib.h |  4 ++--
 3 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 689d256544c8..5c94e74293ce 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -501,13 +501,17 @@ static int ioreq_server_alloc_rangesets(struct ioreq_server *s,
 
     for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
     {
-        char *name;
+        char *name, *type;
 
-        rc = asprintf(&name, "ioreq_server %d %s", id,
-                      (i == XEN_DMOP_IO_RANGE_PORT) ? "port" :
-                      (i == XEN_DMOP_IO_RANGE_MEMORY) ? "memory" :
-                      (i == XEN_DMOP_IO_RANGE_PCI) ? "pci" :
-                      "");
+        switch ( i )
+        {
+        case XEN_DMOP_IO_RANGE_PORT:   type = " port";   break;
+        case XEN_DMOP_IO_RANGE_MEMORY: type = " memory"; break;
+        case XEN_DMOP_IO_RANGE_PCI:    type = " pci";    break;
+        default:                       type = "";        break;
+        }
+
+        rc = xasprintf(&name, "ioreq_server %d%s", id, type);
         if ( rc )
             goto fail;
 
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 185a4bd5610a..b278961cc387 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -859,7 +859,7 @@ int scnprintf(char * buf, size_t size, const char *fmt, ...)
 EXPORT_SYMBOL(scnprintf);
 
 /**
- * vasprintf - Format a string and allocate a buffer to place it in
+ * xvasprintf - Format a string and allocate a buffer to place it in
  *
  * @bufp: Pointer to a pointer to receive the allocated buffer
  * @fmt: The format string to use
@@ -870,7 +870,7 @@ EXPORT_SYMBOL(scnprintf);
  * guaranteed to be null terminated. The memory is allocated
  * from xenheap, so the buffer should be freed with xfree().
  */
-int vasprintf(char **bufp, const char *fmt, va_list args)
+int xvasprintf(char **bufp, const char *fmt, va_list args)
 {
     va_list args_copy;
     size_t size;
@@ -891,7 +891,7 @@ int vasprintf(char **bufp, const char *fmt, va_list args)
 }
 
 /**
- * asprintf - Format a string and place it in a buffer
+ * xasprintf - Format a string and place it in a buffer
  * @bufp: Pointer to a pointer to receive the allocated buffer
  * @fmt: The format string to use
  * @...: Arguments for the format string
@@ -901,14 +901,15 @@ int vasprintf(char **bufp, const char *fmt, va_list args)
  * guaranteed to be null terminated. The memory is allocated
  * from xenheap, so the buffer should be freed with xfree().
  */
-int asprintf(char **bufp, const char *fmt, ...)
+int xasprintf(char **bufp, const char *fmt, ...)
 {
     va_list args;
     int i;
 
     va_start(args, fmt);
-    i=vasprintf(bufp,fmt,args);
+    i = xvasprintf(bufp, fmt, args);
     va_end(args);
+
     return i;
 }
 
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index c6987973bf88..aea60d292724 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -158,9 +158,9 @@ extern int scnprintf(char * buf, size_t size, const char * fmt, ...)
     __attribute__ ((format (printf, 3, 4)));
 extern int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
     __attribute__ ((format (printf, 3, 0)));
-extern int asprintf(char ** bufp, const char * fmt, ...)
+extern int xasprintf(char **bufp, const char *fmt, ...)
     __attribute__ ((format (printf, 2, 3)));
-extern int vasprintf(char ** bufp, const char * fmt, va_list args)
+extern int xvasprintf(char **bufp, const char *fmt, va_list args)
     __attribute__ ((format (printf, 2, 0)));
 
 long simple_strtol(
-- 
2.11.0

On 21.02.2022 11:02, Andrew Cooper wrote:
> Coverity reports that there is a memory leak in
> ioreq_server_alloc_rangesets().  This would be true if Xen's implementation of
> asprintf() had glibc's return semantics, but it doesn't.
> 
> Rename to xasprintf() to reduce confusion for Coverity and other developers.
> 
> While at it, fix style issues.  Rearrange ioreq_server_alloc_rangesets() to
> use a tabulated switch statement, and not to have a trailing space in the
> rangeset name for an unknown range type.
> 
> Coverity-ID: 1472735
> Coverity-ID: 1500265
> Fixes: 780e918a2e54 ("add an implentation of asprintf() for xen")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

On Mon, Feb 21, 2022 at 10:02:53AM +0000, Andrew Cooper wrote:
> Coverity reports that there is a memory leak in
> ioreq_server_alloc_rangesets().  This would be true if Xen's implementation of
> asprintf() had glibc's return semantics, but it doesn't.
> 
> Rename to xasprintf() to reduce confusion for Coverity and other developers.

It would seem more natural to me to rename to asprintk. I assume
there's no way for Coverity to prevent overrides with builtin models?

I've been searching, but there doesn't seem to be any option to
prevent overrides by builtin models?

Thanks, Roger.

On 21/02/2022 11:21, Roger Pau Monné wrote:
> On Mon, Feb 21, 2022 at 10:02:53AM +0000, Andrew Cooper wrote:
>> Coverity reports that there is a memory leak in
>> ioreq_server_alloc_rangesets().  This would be true if Xen's implementation of
>> asprintf() had glibc's return semantics, but it doesn't.
>>
>> Rename to xasprintf() to reduce confusion for Coverity and other developers.
> It would seem more natural to me to rename to asprintk.

Why?  This infrastructure doesn't emit the string to any console.

>  I assume
> there's no way for Coverity to prevent overrides with builtin models?
>
> I've been searching, but there doesn't seem to be any option to
> prevent overrides by builtin models?

No, and we absolutely wouldn't want to skip the model even if we could,
because that would break asprintf() analysis for userspace.

~Andrew