src/util/virobject.h | 5 ++++- src/util/virobject.c | 8 +++++--- 2 files changed, 9 insertions(+), 4 deletions(-)
Checking that the derived class is larger than the requested parent
class saves us from some obvious mistakes, but as written, it does not
catch all the cases; in particular, it is easy to forget to update a
VIR_CLASS_NEW when changing the 'parent' member from virObject to
virObjectLockabale, but where the size checks don't catch that. Add a
parameter for one more layer of sanity checking.
Note that I did NOT change the fact that we require derived classes to
be larger (as the difference in size makes it easy to tell classes
apart), which means that even if a derived class has no functionality
to add (but rather exists for compiler-enforced type-safety), it must
still include a dummy member. But I did fix the wording of the error
message to match the code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Here's hoping Coverity doesn't have a false-positive complaint about
the error message being a potential dereference of NULL (the only time
'parent == NULL' is when 'parentsize == 0', based on the fact that our
syntax checks forbid raw calls to virClassNew() except for "virObject"
itself - but Coverity likely won't see that).
src/util/virobject.h | 5 ++++-
src/util/virobject.c | 8 +++++---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/util/virobject.h b/src/util/virobject.h
index d4ec943a43..757068fcc1 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -82,12 +82,15 @@ virClassPtr virClassForObjectRWLockable(void);
*/
# define VIR_CLASS_NEW(name, prnt) \
verify_expr(offsetof(name, parent) == 0, \
- (name##Class = virClassNew(prnt, #name, sizeof(name), name##Dispose)))
+ (name##Class = virClassNew(prnt, #name, sizeof(name), \
+ sizeof(((name *)NULL)->parent), \
+ name##Dispose)))
virClassPtr
virClassNew(virClassPtr parent,
const char *name,
size_t objectSize,
+ size_t parentSize,
virObjectDisposeCallback dispose)
VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(2);
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 3b28331ba7..b4ee068cb2 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -78,6 +78,7 @@ virObjectOnceInit(void)
if (!(virObjectClass = virClassNew(NULL,
"virObject",
sizeof(virObject),
+ 0,
NULL)))
return -1;
@@ -159,6 +160,7 @@ virClassPtr
virClassNew(virClassPtr parent,
const char *name,
size_t objectSize,
+ size_t parentSize,
virObjectDisposeCallback dispose)
{
virClassPtr klass;
@@ -167,10 +169,10 @@ virClassNew(virClassPtr parent,
STRNEQ(name, "virObject")) {
virReportInvalidNonNullArg(parent);
return NULL;
- } else if (parent &&
- objectSize <= parent->objectSize) {
+ } else if (objectSize <= parentSize ||
+ parentSize != (parent ? parent->objectSize : 0)) {
virReportInvalidArg(objectSize,
- _("object size %zu of %s is smaller than parent class %zu"),
+ _("object size %zu of %s is not larger than parent class %zu"),
objectSize, name, parent->objectSize);
return NULL;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 15, 2019 at 10:12:45AM -0500, Eric Blake wrote: > Checking that the derived class is larger than the requested parent > class saves us from some obvious mistakes, but as written, it does not > catch all the cases; in particular, it is easy to forget to update a > VIR_CLASS_NEW when changing the 'parent' member from virObject to > virObjectLockabale, but where the size checks don't catch that. Add a > parameter for one more layer of sanity checking. > > Note that I did NOT change the fact that we require derived classes to > be larger (as the difference in size makes it easy to tell classes > apart), which means that even if a derived class has no functionality > to add (but rather exists for compiler-enforced type-safety), it must > still include a dummy member. But I did fix the wording of the error > message to match the code. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 15, 2019 at 10:12:45AM -0500, Eric Blake wrote: >Checking that the derived class is larger than the requested parent >class saves us from some obvious mistakes, but as written, it does not >catch all the cases; in particular, it is easy to forget to update a >VIR_CLASS_NEW when changing the 'parent' member from virObject to >virObjectLockabale, but where the size checks don't catch that. Add a >parameter for one more layer of sanity checking. > >Note that I did NOT change the fact that we require derived classes to >be larger (as the difference in size makes it easy to tell classes >apart), which means that even if a derived class has no functionality >to add (but rather exists for compiler-enforced type-safety), it must >still include a dummy member. But I did fix the wording of the error >message to match the code. > >Signed-off-by: Eric Blake <eblake@redhat.com> >--- > >Here's hoping Coverity doesn't have a false-positive complaint about >the error message being a potential dereference of NULL (the only time >'parent == NULL' is when 'parentsize == 0', based on the fact that our >syntax checks forbid raw calls to virClassNew() except for "virObject" >itself - but Coverity likely won't see that). > > src/util/virobject.h | 5 ++++- > src/util/virobject.c | 8 +++++--- > 2 files changed, 9 insertions(+), 4 deletions(-) > Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 3/15/19 10:30 AM, Ján Tomko wrote: > On Fri, Mar 15, 2019 at 10:12:45AM -0500, Eric Blake wrote: >> Checking that the derived class is larger than the requested parent >> class saves us from some obvious mistakes, but as written, it does not >> catch all the cases; in particular, it is easy to forget to update a >> VIR_CLASS_NEW when changing the 'parent' member from virObject to >> virObjectLockabale, but where the size checks don't catch that. Add a >> parameter for one more layer of sanity checking. >> >> Note that I did NOT change the fact that we require derived classes to >> be larger (as the difference in size makes it easy to tell classes >> apart), which means that even if a derived class has no functionality >> to add (but rather exists for compiler-enforced type-safety), it must >> still include a dummy member. But I did fix the wording of the error >> message to match the code. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> >> Here's hoping Coverity doesn't have a false-positive complaint about >> the error message being a potential dereference of NULL (the only time >> 'parent == NULL' is when 'parentsize == 0', based on the fact that our >> syntax checks forbid raw calls to virClassNew() except for "virObject" >> itself - but Coverity likely won't see that). Actually, I realized we have sa_assert() for that. >> >> src/util/virobject.h | 5 ++++- >> src/util/virobject.c | 8 +++++--- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> > > Reviewed-by: Ján Tomko <jtomko@redhat.com> Thanks; tweaked and pushed. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.