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

Eric Blake posted 1 patch 5 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 5 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 5 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 5 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é 5 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