| Message ID | 20220221100254.13661-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc coverity fixes and tweaks | expand |
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
On Mon, Feb 21, 2022 at 11:28:39AM +0000, Andrew Cooper wrote: > 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. Right, but the f in printf is for print formatted, not for where the output is supposed to go. So printk is the outlier and should instead be kprintf? I can buy into using xasprintf (also because that's what Linux does with kasprintf), but I don't think it's so obvious given the precedent of having printk instead of printf. > > 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. Well, we could maybe find a way to only enable the flag for hypervisor code build, but anyway, it's pointless to discus if there's no flag in the first place. Coverity could be clever enough to check if there's an implementation provided for those, instead of unconditionally override with a model. Thanks, Roger.
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 21/02/2022 12:23, Roger Pau Monne wrote: > On Mon, Feb 21, 2022 at 11:28:39AM +0000, Andrew Cooper wrote: >> 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. > Right, but the f in printf is for print formatted, not for where the > output is supposed to go. So printk is the outlier and should instead > be kprintf? > > I can buy into using xasprintf (also because that's what Linux does > with kasprintf), but I don't think it's so obvious given the precedent > of having printk instead of printf. The naming isn't ideal, but this is Xen's local version of the thing called asprintf() in userspace. Naming it anything other than xasprintf() is going to be even more confusing for people than this mess already is... >>> 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. > Well, we could maybe find a way to only enable the flag for hypervisor > code build, but anyway, it's pointless to discus if there's no flag in > the first place. > > Coverity could be clever enough to check if there's an implementation > provided for those, instead of unconditionally override with a > model. There is no way to disable the model for asprintf(). ~Andrew
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(
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(-)