[libvirt] [PATCH] object: Add sanity check on correct parent class

Eric Blake posted 1 patch 13 weeks ago
Failed in applying to current master (apply log)
src/util/virobject.h | 5 ++++-
src/util/virobject.c | 8 +++++---
2 files changed, 9 insertions(+), 4 deletions(-)

[libvirt] [PATCH] object: Add sanity check on correct parent class

Posted by Eric Blake 13 weeks ago
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

Re: [libvirt] [PATCH] object: Add sanity check on correct parent class

Posted by Ján Tomko 13 weeks ago
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

Re: [libvirt] [PATCH] object: Add sanity check on correct parent class

Posted by Eric Blake 13 weeks ago
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

Re: [libvirt] [PATCH] object: Add sanity check on correct parent class

Posted by Daniel P. Berrangé 13 weeks ago
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