On Fri, Feb 12, 2021 at 12:14:27PM +0100, Michal Privoznik wrote: > 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. I agree - it is, especially now that GCC employs a static analyzer it should be smart and not optimize it away, not sure whether the memset optimization happened only with GCC or was a generic thing hence I won't try to make any comments about Clang. Erik
