10:04 a.m.
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
10:04 a.m.
New subject: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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(a)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
8:18 a.m.
New subject: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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
7:10 a.m.
New subject: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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.
7:30 a.m.
New subject: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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 :|
8:03 a.m.
New subject: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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.
8:15 a.m.
New subject: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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 :|
10:04 a.m.
New subject: [libvirt] [PATCH 2/4] util: alloc: Introduce macro for self-freeing NULL-terminated lists
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(a)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
9:50 a.m.
New subject: [libvirt] [PATCH 2/4] util: alloc: Introduce macro for self-freeing NULL-terminated lists
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(a)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
10:04 a.m.
New subject: [libvirt] [PATCH 3/4] util: string: Use VIR_AUTOLISTPTR for 'virString'
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(a)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
10:04 a.m.
New subject: [libvirt] [PATCH 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
We'd free only the first element of the vector leaking the rest.
Signed-off-by: Peter Krempa <pkrempa(a)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
2097
days inactive
2101
days old
10 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Peter Krempa