Re: [libvirt] [PATCH v2 4/8] util: Add safety net of checks to ensure valid object

On Fri, Jun 02, 2017 at 06:17:18 -0400, John Ferlan wrote: > The virObject logic "assumes" that whatever is passed to its API's > would be some sort of virObjectPtr; however, if it is not then some > really bad things can happen. > > So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock}, > and virObjectIsClass and the virObject and virObjectLockable class > consumers have been well behaved and code well tested. Soon there will > be more consumers and one such consumer tripped over this during testing > by passing a virHashTablePtr to virObjectIsClass which ends up calling > virClassIsDerivedFrom using "obj->klass", which wasn't really a klass > object causing one of those bad things to happen. > > To avoid the future possibility that a non virObject class memory was > passed to some virObject* API, this patch adds two new checks - one > to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic > and the other to ensure obj->u.s.magic doesn't "wrap" some day to > 0xCAFF0000 if we ever get that many objects. > > It is still left up to the caller to handle the failed API calls just > as it would be if it passed a NULL opaque pointer anyobj. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/util/virobject.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/util/virobject.c b/src/util/virobject.c > index 9f5f187..e0465b1 100644 > --- a/src/util/virobject.c > +++ b/src/util/virobject.c [..] > @@ -156,6 +159,7 @@ virClassNew(virClassPtr parent, > if (VIR_STRDUP(klass->name, name) < 0) > goto error; > klass->magic = virAtomicIntInc(&magicCounter); > + assert(klass->magic <= 0xCAFEFFFF); Library code should not use assert. This makes the client application crash on library usage. This also does not deterministically make this crash all the time. Only if you initialize the class that exceeds the marker, which depends on serialization of the initialization.