[libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions

[Daniel P. Berrangé]

On Fri, Feb 12, 2021 at 11:07:21AM +0100, Erik Skultety wrote:
> On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote:
> > On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote:
> > > I've looked at a few of these, and one thing I've found is that very
> > > often we have a function called somethingSomethingClear(), and:
> > > 
> > > 1) The only places it is ever called will immediately free the memory
> > > of the object as soon as they clear it.
> > > 
> > > and very possibly
> > > 
> > > 2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but then some of the other pointers call
> > > 
> > >    bobLoblawFree(def->bobloblaw)
> > > 
> > > and then don't actually set def->bobloblaw to NULL - so the functions
> > > aren't actually "Clearing", they're "Freeing and then clearing a few
> > > things, but not everything".
> > > 
> > 
> > One thing I am wondering is whether this is really only used where it makes
> > sense.  As far as I understand, and please correct me if I am way off, the
> > purpose of the Clear functions is to:
> > 
> >  a) provide a way to remove everything from a structure that the current
> >     function cannot recreate (there is a pointer to it somewhere else which
> >     would not be updated) and
> > 
> >  b) provide a way to reset a structure so that it can be filled again without
> >     needless reallocation.
> > 
> > I think (b) is obviously pointless, especially lately, so the only reasonable
> > usage would be for the scenario (a).  However, I think I remember this also
> > being used in places where it would be perfectly fine to free the variable and
> > recreate it.  Maybe it could ease up the decision, at least by eliminating some
> > of the code, if my hunch is correct.
> > 
> > In my quick search I only found virDomainVideoDefClear to be used in this manner
> > and I am not convinced that it is worth having this extra function with extra
> 
> You could always memset it explicitly, someone might find the code more
> readable then. IMO I'd vote for an explicit memset just for the sake of better
> security practice (since we'll have to wait a little while for something like
> SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how
> many cycles exactly would be wasted, but IIRC a recent discussion memset can be
> optimized away (correct me if I don't remember it well!), so Dan P.B.
> suggested to gradually convert to some platform-specific ways on how to
> sanitize the memory safely - with that in mind, I'd say we use an explicit
> memset in all the functions in question and convert them later?

I only suggest that for places where security is required. ie to scrub
passwords.

If the compiler wants to cull memsets in places unrelated to security
that's fine by me, or at least, not our problem to worry about.

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 :|