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

[Michal Privoznik]

On 2/12/21 11:07 AM, 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 think one can argue that if there's a memset() called inside a 
function that is supposed to clear out a member of a struct and later 
the member is tested against NULL, but compiler decides to "optimize" 
memset out - it's a compiler bug.

Michal