[libvirt] [PATCH 0/4] util: Few more VIR_AUTO... related cleanups
Document that VIR_AUTOPTR should not be used with vectors ... Peter Krempa (4): util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs util: alloc: Introduce macro for self-freeing NULL-terminated lists util: string: Use VIR_AUTOLISTPTR for 'virString' util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors src/lxc/lxc_process.c | 2 +- src/qemu/qemu_conf.c | 8 +-- src/storage/storage_backend_sheepdog.c | 4 +- src/storage/storage_backend_zfs.c | 10 ++-- src/util/viralloc.h | 70 +++++++++++++++++++++++++- src/util/vircommand.c | 2 +- src/util/virfirewall.c | 2 +- src/util/virprocess.c | 2 +- src/util/virstoragefile.c | 10 ++-- src/util/virstring.h | 2 +- src/xenconfig/xen_common.c | 6 +-- 11 files changed, 93 insertions(+), 25 deletions(-) -- 2.20.1
VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the resources for the given VIR_AUTOPTR variable. Given that the cleanup function is executed when the stack frame is being destroyed it does not make much sense to set the pointer to NULL. Making the inline function contain less code also decreases the possibility that for a given type the inlining will be declined by the compiler due to increasing code size. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 15451d4673..50a07d4fa3 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -612,7 +612,6 @@ void virAllocTestHook(void (*func)(int, void*), void *data); { \ if (*_ptr) \ (func)(*_ptr); \ - *_ptr = NULL; \ } # define VIR_AUTOCLEAN_FUNC_NAME(type) type##AutoClean -- 2.20.1
On Fri, Feb 22, 2019 at 05:04:37PM +0100, Peter Krempa wrote:
VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the resources for the given VIR_AUTOPTR variable. Given that the cleanup function is executed when the stack frame is being destroyed it does not make much sense to set the pointer to NULL.
Although correct, the current code is correct too and I really don't see any benefit behind removing a single line where you reset a variable to NULL, if anything, this adds more safety regardless of scenario (defensive programming in general). Originally, I wanted to argue, that this might introduce an issue in loops if you pass the temporary variable to a function by reference and then exiting on failure would free some garbage, but that's irrelevant since Sukrit introduced a syntax check to ensure, VIR_AUTO variables are always initialized.
Making the inline function contain less code also decreases the possibility that for a given type the inlining will be declined by the compiler due to increasing code size.
I honestly doubt that re-setting a variable to NULL will have any impact on this and it's such a trivial operation that the compiler might optimize this for you in some way. I think this patch should be dropped from the series and left as is. Erik
On Mon, Feb 25, 2019 at 15:18:53 +0100, Erik Skultety wrote:
On Fri, Feb 22, 2019 at 05:04:37PM +0100, Peter Krempa wrote:
VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the resources for the given VIR_AUTOPTR variable. Given that the cleanup function is executed when the stack frame is being destroyed it does not make much sense to set the pointer to NULL.
Although correct, the current code is correct too and I really don't see any benefit behind removing a single line where you reset a variable to NULL, if anything, this adds more safety regardless of scenario (defensive programming in general).
Well I'm not against defensive tactics in programming but resetting a a pointer value to NULL when the pointer is leaving scope is almost as useful as using a hard hat to prevent drowning in a pool. There is no way to use the variable which would not be a completely different class of bug. Also in no way does this keep the stack "tidy". All stacked variables must still be considered uninitialized even if we'd make sure that everything resets it's stack copies. The only thing that comes into my mind where this would "save" us is if you assign a pointer of a pointer defined by VIR_AUTOPTR to a variable which also has a __attribute(cleanup... type handler which would dereference it. That is also wrong on the basis that we should never rely on the order those cleanup functions are executed even if the standard does specify it. Please don't mix this up with what VIR_FREE is doing. Resetting pointers in VIR_FREE prevents bugs as the pointer is still a valid variable.
Originally, I wanted to argue, that this might introduce an issue in loops if you pass the temporary variable to a function by reference and then exiting on failure would free some garbage, but that's irrelevant since Sukrit introduced a syntax check to ensure, VIR_AUTO variables are always initialized.
Making the inline function contain less code also decreases the possibility that for a given type the inlining will be declined by the compiler due to increasing code size.
I honestly doubt that re-setting a variable to NULL will have any impact on this and it's such a trivial operation that the compiler might optimize this for you in some way.
Well, I unfortunately can't seem to be able to reproduce this claim at this point. The compiled code does have the extra instruction which clears the variable.
On Tue, Feb 26, 2019 at 02:10:02PM +0100, Peter Krempa wrote:
On Mon, Feb 25, 2019 at 15:18:53 +0100, Erik Skultety wrote:
On Fri, Feb 22, 2019 at 05:04:37PM +0100, Peter Krempa wrote:
VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the resources for the given VIR_AUTOPTR variable. Given that the cleanup function is executed when the stack frame is being destroyed it does not make much sense to set the pointer to NULL.
Although correct, the current code is correct too and I really don't see any benefit behind removing a single line where you reset a variable to NULL, if anything, this adds more safety regardless of scenario (defensive programming in general).
Well I'm not against defensive tactics in programming but resetting a a pointer value to NULL when the pointer is leaving scope is almost as useful as using a hard hat to prevent drowning in a pool.
There is no way to use the variable which would not be a completely different class of bug. Also in no way does this keep the stack "tidy". All stacked variables must still be considered uninitialized even if we'd make sure that everything resets it's stack copies.
In "normal" execution setting the variable to NULL is indeed useless but the benefit of defensive programming comes in the abnormal execution scenarios. The stack region being cleared in the current function is the same chunk of memory that will be used for stack in the next function the caller invokes. Having the stack littered with data from variables that are now out of scope is very useful to people exploiting security bugs. Zero'ing out pointers and memory is a worthwhile thing todo even if they're going out of scope. Stack corruption is common in C as it is a memory-unsafe language, so any steps we can take to provide a cleaner state to the stack is useful if it doesn't cost us performance. So I agree with Erik that we shouldn't remove these kind of assignments, even if they are "dead code" during normal execution codepaths. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Feb 26, 2019 at 13:30:55 +0000, Daniel Berrange wrote:
On Tue, Feb 26, 2019 at 02:10:02PM +0100, Peter Krempa wrote:
On Mon, Feb 25, 2019 at 15:18:53 +0100, Erik Skultety wrote:
On Fri, Feb 22, 2019 at 05:04:37PM +0100, Peter Krempa wrote:
VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the resources for the given VIR_AUTOPTR variable. Given that the cleanup function is executed when the stack frame is being destroyed it does not make much sense to set the pointer to NULL.
Although correct, the current code is correct too and I really don't see any benefit behind removing a single line where you reset a variable to NULL, if anything, this adds more safety regardless of scenario (defensive programming in general).
Well I'm not against defensive tactics in programming but resetting a a pointer value to NULL when the pointer is leaving scope is almost as useful as using a hard hat to prevent drowning in a pool.
There is no way to use the variable which would not be a completely different class of bug. Also in no way does this keep the stack "tidy". All stacked variables must still be considered uninitialized even if we'd make sure that everything resets it's stack copies.
In "normal" execution setting the variable to NULL is indeed useless but the benefit of defensive programming comes in the abnormal execution scenarios. The stack region being cleared in the current function is the same chunk of memory that will be used for stack in the next function the caller invokes. Having the stack littered with data from variables that are now out of scope is very useful to people exploiting security bugs. Zero'ing out pointers and memory is a worthwhile thing todo even
I will not buy that this is a security thing as long as VIR_AUTOFREE and VIR_FREE are a thing. It sounds great that we try to zero out the pointers but if we keep the values on the heap it does not buy that much. Additionally we don't do this for any temp pointer which may point into the same memory.
if they're going out of scope. Stack corruption is common in C as it is a memory-unsafe language, so any steps we can take to provide a cleaner state to the stack is useful if it doesn't cost us performance. So I agree with Erik that we shouldn't remove these kind of assignments, even if they are "dead code" during normal execution codepaths.
On Tue, Feb 26, 2019 at 03:03:01PM +0100, Peter Krempa wrote:
On Tue, Feb 26, 2019 at 13:30:55 +0000, Daniel Berrange wrote:
On Tue, Feb 26, 2019 at 02:10:02PM +0100, Peter Krempa wrote:
On Mon, Feb 25, 2019 at 15:18:53 +0100, Erik Skultety wrote:
On Fri, Feb 22, 2019 at 05:04:37PM +0100, Peter Krempa wrote:
VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the resources for the given VIR_AUTOPTR variable. Given that the cleanup function is executed when the stack frame is being destroyed it does not make much sense to set the pointer to NULL.
Although correct, the current code is correct too and I really don't see any benefit behind removing a single line where you reset a variable to NULL, if anything, this adds more safety regardless of scenario (defensive programming in general).
Well I'm not against defensive tactics in programming but resetting a a pointer value to NULL when the pointer is leaving scope is almost as useful as using a hard hat to prevent drowning in a pool.
There is no way to use the variable which would not be a completely different class of bug. Also in no way does this keep the stack "tidy". All stacked variables must still be considered uninitialized even if we'd make sure that everything resets it's stack copies.
In "normal" execution setting the variable to NULL is indeed useless but the benefit of defensive programming comes in the abnormal execution scenarios. The stack region being cleared in the current function is the same chunk of memory that will be used for stack in the next function the caller invokes. Having the stack littered with data from variables that are now out of scope is very useful to people exploiting security bugs. Zero'ing out pointers and memory is a worthwhile thing todo even
I will not buy that this is a security thing as long as VIR_AUTOFREE and VIR_FREE are a thing. It sounds great that we try to zero out the pointers but if we keep the values on the heap it does not buy that much.
VIR_FREE works the same way as this code does - it zeros out the pointer to the pointer after free'ing it. The VIR_DEFINE_AUTOPTR_FUNC macro is ultimately a hack to deal with the fact that most other *Free APIs are just taking a pointer arg, not a pointer to a pointer. When we introduced that I requested that we actally change the other APIs to take a pointer to a pointer, so that they would nullify the the pointer after free'ing it, just like VIR_FREE does. This VIR_DEFINE_AUTOPTR_FUNC does the nullify so that this works the way we'd ultimately like all the *Free methods to work. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
VIR_AUTOLISTPTR allows you to define a pointer to a NULL-terminated list of elements which will be auto-freed when destroying the scope. This is done so that we can avoid using VIR_AUTOPTR in those cases as it worked only for virStringList-related stuff. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 61 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 50a07d4fa3..983a6e83d1 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -631,6 +631,54 @@ void virAllocTestHook(void (*func)(int, void*), void *data); (func)(_ptr); \ } + +# define VIR_AUTOLISTPTR_FUNC_NAME(type) type##AutoListPtrFree + +/** + * VIR_DEFINE_AUTOLISTPTR_FUNC: + * @type: type of the element of the list + * @func: freeing function for a single element of the list + * + * This macro defines the freeing function used by VIR_AUTOLISTPTR for @type. + * @func must free one of the elements of @type. + * + * Note that the function is not inline due to size and thus + * VIR_DECLARE_AUTOLISTPTR_FUNC needs to be used in the appropriate header file. + * + * VIR_DEFINE_AUTOLISTPTR_FUNC is mutually exclusive with + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT. + */ +# define VIR_DECLARE_AUTOLISTPTR_FUNC(type) \ + void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr); +# define VIR_DEFINE_AUTOLISTPTR_FUNC(type, func) \ + void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr) \ + { \ + type *n;\ + for (n = *_ptr; n && *n; n++) \ + (func)(n); \ + VIR_FREE(*_ptr);\ + } + +/** + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT: + * @type: type of the element of the list + * @func: freeing function for the whole NULL-terminated list of @type + * + * This macro defines the freeing function used by VIR_AUTOLISTPTR for @type. + * @func must free a NULL terminated dense list of @type and also the list + * itself. This is a convenience option if @type has already such function. + * + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT is mutually exclusive with + * VIR_DEFINE_AUTOLISTPTR_FUNC. + */ +# define VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT(type, func) \ + static inline void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr) \ + { \ + if (*_ptr) \ + (func)(*_ptr); \ + } + + /** * VIR_AUTOFREE: * @type: type of the variable to be freed automatically @@ -665,6 +713,19 @@ void virAllocTestHook(void (*func)(int, void*), void *data); # define VIR_AUTOCLEAN(type) \ __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type +/** + * VIR_AUTOLISTPTR: + * @type: type of the members of the self-freeing NULL-terminated list to be defined + * + * This macro defines a pointer to a NULL-terminated list of @type members. The + * list is automatically freed when it goes out of scope including elements + * themselves. + * + * The freeing function is registered by VIR_DEFINE_AUTOLISTPTR_FUNC or + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT macro for the given type. + */ +# define VIR_AUTOLISTPTR(type) \ + __attribute__((cleanup(VIR_AUTOLISTPTR_FUNC_NAME(type)))) type * /** * VIR_AUTOUNREF: -- 2.20.1
On Fri, Feb 22, 2019 at 05:04:38PM +0100, Peter Krempa wrote:
VIR_AUTOLISTPTR allows you to define a pointer to a NULL-terminated list of elements which will be auto-freed when destroying the scope.
This is done so that we can avoid using VIR_AUTOPTR in those cases as it worked only for virStringList-related stuff.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 61 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 50a07d4fa3..983a6e83d1 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -631,6 +631,54 @@ void virAllocTestHook(void (*func)(int, void*), void *data); (func)(_ptr); \ }
+ +# define VIR_AUTOLISTPTR_FUNC_NAME(type) type##AutoListPtrFree + +/** + * VIR_DEFINE_AUTOLISTPTR_FUNC: + * @type: type of the element of the list + * @func: freeing function for a single element of the list + * + * This macro defines the freeing function used by VIR_AUTOLISTPTR for @type. + * @func must free one of the elements of @type. + * + * Note that the function is not inline due to size and thus + * VIR_DECLARE_AUTOLISTPTR_FUNC needs to be used in the appropriate header file. + * + * VIR_DEFINE_AUTOLISTPTR_FUNC is mutually exclusive with + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT. + */ +# define VIR_DECLARE_AUTOLISTPTR_FUNC(type) \ + void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr); +# define VIR_DEFINE_AUTOLISTPTR_FUNC(type, func) \ + void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr) \ + { \ + type *n;\ + for (n = *_ptr; n && *n; n++) \ + (func)(n); \ + VIR_FREE(*_ptr);\ + }
So, I believe that ^this should be unnecessary and it should always be handled by the function. I don't have a good argument yet, I need to think about this some more, but at the time being, I think that if we have a type from which we create a NULL-terminated list, we should have dedicated type for that too and a destructor as well, ergo VIR_AUTOPTR suffices. I appreciate and understand the effort, solely because it was virString driving it which is confusing on its own because we can't make AUTOPTR work properly because virString introduces an additional pointer and everything would go haywire if we had virStringList type. Maybe it would be worth saying that virString is indeed special and therefore needs to be handled on its own with its own macro. That way, the whole AUTOLIST wouldn't be needed, at least not in the current form. I can be convinced otherwise with good arguments though.
+ +/** + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT: + * @type: type of the element of the list + * @func: freeing function for the whole NULL-terminated list of @type + * + * This macro defines the freeing function used by VIR_AUTOLISTPTR for @type. + * @func must free a NULL terminated dense list of @type and also the list + * itself. This is a convenience option if @type has already such function. + * + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT is mutually exclusive with + * VIR_DEFINE_AUTOLISTPTR_FUNC. + */ +# define VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT(type, func) \ + static inline void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr) \ + { \ + if (*_ptr) \ + (func)(*_ptr); \ + }
So the only difference between ^this and VIR_AUTOPTR is the description. Erik
VIR_AUTOPTR should not be used for vectors except for the rare case of NULL-terminated lists. Convert all cases of usage of VIR_AUTOPTR for 'virString' to VIR_AUTOLISTPTR. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_conf.c | 8 ++++---- src/storage/storage_backend_sheepdog.c | 4 ++-- src/storage/storage_backend_zfs.c | 10 +++++----- src/util/vircommand.c | 2 +- src/util/virfirewall.c | 2 +- src/util/virprocess.c | 2 +- src/util/virstoragefile.c | 10 +++++----- src/util/virstring.h | 2 +- src/xenconfig/xen_common.c | 6 +++--- 10 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index a3481bfa08..566ed13fda 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1181,7 +1181,7 @@ int virLXCProcessStart(virConnectPtr conn, size_t i; char *logfile = NULL; int logfd = -1; - VIR_AUTOPTR(virString) veths = NULL; + VIR_AUTOLISTPTR(virString) veths = NULL; int handshakefds[2] = { -1, -1 }; off_t pos = -1; char ebuf[1024]; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2f5ef8d0c4..72a0b97772 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -645,7 +645,7 @@ static int virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) { - VIR_AUTOPTR(virString) hugetlbfs = NULL; + VIR_AUTOLISTPTR(virString) hugetlbfs = NULL; VIR_AUTOFREE(char *) stdioHandler = NULL; VIR_AUTOFREE(char *) corestr = NULL; size_t i; @@ -832,7 +832,7 @@ static int virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) { - VIR_AUTOPTR(virString) nvram = NULL; + VIR_AUTOLISTPTR(virString) nvram = NULL; size_t i; if (virConfGetValueStringList(conf, "nvram", false, &nvram) < 0) @@ -869,8 +869,8 @@ virQEMUDriverConfigLoadSecurityEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf, bool privileged) { - VIR_AUTOPTR(virString) controllers = NULL; - VIR_AUTOPTR(virString) namespaces = NULL; + VIR_AUTOLISTPTR(virString) controllers = NULL; + VIR_AUTOLISTPTR(virString) namespaces = NULL; VIR_AUTOFREE(char *) user = NULL; VIR_AUTOFREE(char *) group = NULL; size_t i, j; diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 99f3283a1c..4c01207409 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -138,8 +138,8 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) { size_t i; VIR_AUTOFREE(char *) output = NULL; - VIR_AUTOPTR(virString) lines = NULL; - VIR_AUTOPTR(virString) cells = NULL; + VIR_AUTOLISTPTR(virString) lines = NULL; + VIR_AUTOLISTPTR(virString) cells = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", "-r", NULL); diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index 7ffdff638e..25f9e90f98 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -106,8 +106,8 @@ virStorageBackendZFSParseVol(virStoragePoolObjPtr pool, bool is_new_vol = false; virStorageVolDefPtr volume = NULL; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - VIR_AUTOPTR(virString) tokens = NULL; - VIR_AUTOPTR(virString) name_tokens = NULL; + VIR_AUTOLISTPTR(virString) tokens = NULL; + VIR_AUTOLISTPTR(virString) name_tokens = NULL; if (!(tokens = virStringSplitCount(volume_string, "\t", 0, &count))) return -1; @@ -177,7 +177,7 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool, { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); size_t i; - VIR_AUTOPTR(virString) lines = NULL; + VIR_AUTOLISTPTR(virString) lines = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOFREE(char *) volumes_list = NULL; @@ -224,8 +224,8 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED) char *zpool_props = NULL; size_t i; VIR_AUTOPTR(virCommand) cmd = NULL; - VIR_AUTOPTR(virString) lines = NULL; - VIR_AUTOPTR(virString) tokens = NULL; + VIR_AUTOLISTPTR(virString) lines = NULL; + VIR_AUTOLISTPTR(virString) tokens = NULL; /** * $ zpool get -Hp health,size,free,allocated test diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 3d533c68a6..8d8862ad85 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2983,7 +2983,7 @@ virCommandRunRegex(virCommandPtr cmd, int totgroups = 0, ngroup = 0, maxvars = 0; char **groups; VIR_AUTOFREE(char *) outbuf = NULL; - VIR_AUTOPTR(virString) lines = NULL; + VIR_AUTOLISTPTR(virString) lines = NULL; int ret = -1; /* Compile all regular expressions */ diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index d63ce05ed8..2b7620f943 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -719,7 +719,7 @@ virFirewallApplyRule(virFirewallPtr firewall, { VIR_AUTOFREE(char *) output = NULL; VIR_AUTOFREE(char *) str = virFirewallRuleToString(rule); - VIR_AUTOPTR(virString) lines = NULL; + VIR_AUTOLISTPTR(virString) lines = NULL; VIR_INFO("Applying rule '%s'", NULLSTR(str)); if (rule->ignoreErrors) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4e69228f34..80aba1e44d 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -987,7 +987,7 @@ int virProcessGetStartTime(pid_t pid, int len; VIR_AUTOFREE(char *) filename = NULL; VIR_AUTOFREE(char *) buf = NULL; - VIR_AUTOPTR(virString) tokens = NULL; + VIR_AUTOLISTPTR(virString) tokens = NULL; if (virAsprintf(&filename, "/proc/%llu/stat", (long long) pid) < 0) return -1; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b2e308d81d..87ce299e64 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1571,7 +1571,7 @@ virStorageFileParseBackingStoreStr(const char *str, size_t nstrings; unsigned int idx = 0; char *suffix; - VIR_AUTOPTR(virString) strings = NULL; + VIR_AUTOLISTPTR(virString) strings = NULL; *chainIndex = 0; @@ -2661,7 +2661,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, virURIPtr uri = NULL; const char *path = NULL; int ret = -1; - VIR_AUTOPTR(virString) scheme = NULL; + VIR_AUTOLISTPTR(virString) scheme = NULL; if (!(uri = virURIParse(uristr))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2765,7 +2765,7 @@ virStorageSourceRBDAddHost(virStorageSourcePtr src, { char *port; size_t skip; - VIR_AUTOPTR(virString) parts = NULL; + VIR_AUTOLISTPTR(virString) parts = NULL; if (VIR_EXPAND_N(src->hosts, src->nhosts, 1) < 0) return -1; @@ -2921,7 +2921,7 @@ static int virStorageSourceParseNBDColonString(const char *nbdstr, virStorageSourcePtr src) { - VIR_AUTOPTR(virString) backing = NULL; + VIR_AUTOLISTPTR(virString) backing = NULL; if (!(backing = virStringSplit(nbdstr, ":", 0))) return -1; @@ -4208,7 +4208,7 @@ int virStorageFileCheckCompat(const char *compat) { unsigned int result; - VIR_AUTOPTR(virString) version = NULL; + VIR_AUTOLISTPTR(virString) version = NULL; if (!compat) return 0; diff --git a/src/util/virstring.h b/src/util/virstring.h index aef82471c2..b2c7178686 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -307,6 +307,6 @@ int virStringParsePort(const char *str, unsigned int *port) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -VIR_DEFINE_AUTOPTR_FUNC(virString, virStringListFree); +VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT(virString, virStringListFree); #endif /* LIBVIRT_VIRSTRING_H */ diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 94e0703cf3..6b0bdcb178 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -473,7 +473,7 @@ xenHandleConfGetValueStringListErrors(int ret) static int xenParsePCIList(virConfPtr conf, virDomainDefPtr def) { - VIR_AUTOPTR(virString) pcis = NULL; + VIR_AUTOLISTPTR(virString) pcis = NULL; virString *entries = NULL; int rc; @@ -666,7 +666,7 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) } if (!hvm && def->graphics == NULL) { /* New PV guests use this format */ - VIR_AUTOPTR(virString) vfbs = NULL; + VIR_AUTOLISTPTR(virString) vfbs = NULL; int rc; if ((rc = virConfGetValueStringList(conf, "vfb", false, &vfbs)) == 1) { @@ -764,7 +764,7 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) static int xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) { - VIR_AUTOPTR(virString) serials = NULL; + VIR_AUTOLISTPTR(virString) serials = NULL; virDomainChrDefPtr chr = NULL; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { -- 2.20.1
We'd free only the first element of the vector leaking the rest. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 983a6e83d1..6de311259d 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -697,6 +697,10 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * the variable declared with it by calling the function * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable * goes out of scope. + * + * Note that this macro must NOT be used with vectors! The cleaning function + * will not free any elements beyond the first. + * See VIR_AUTOLISTPTR for NULL-terminated lists. */ # define VIR_AUTOPTR(type) \ __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type * @@ -709,6 +713,10 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * when the variable goes out of scope. * The cleanup function is registered by VIR_DEFINE_AUTOCLEAN_FUNC macro for * the given type. + * + * Note that this macro must NOT be used with vectors! The cleaning function + * will not free any elements beyond the first. + * See VIR_AUTOLISTPTR for NULL-terminated lists. */ # define VIR_AUTOCLEAN(type) \ __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type -- 2.20.1
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Peter Krempa