[libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs

[Daniel P. Berrangé]

On Tue, Feb 26, 2019 at 03:03:01PM +0100, Peter Krempa wrote:
> On Tue, Feb 26, 2019 at 13:30:55 +0000, Daniel Berrange wrote:
> > On Tue, Feb 26, 2019 at 02:10:02PM +0100, Peter Krempa wrote:
> > > On Mon, Feb 25, 2019 at 15:18:53 +0100, Erik Skultety wrote:
> > > > On Fri, Feb 22, 2019 at 05:04:37PM +0100, Peter Krempa wrote:
> > > > > VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the
> > > > > resources for the given VIR_AUTOPTR variable. Given that the cleanup
> > > > > function is executed when the stack frame is being destroyed it does not
> > > > > make much sense to set the pointer to NULL.
> > > > 
> > > > Although correct, the current code is correct too and I really don't see any
> > > > benefit behind removing a single line where you reset a variable to NULL, if
> > > > anything, this adds more safety regardless of scenario (defensive programming
> > > > in general).
> > > 
> > > Well I'm not against defensive tactics in programming but resetting a
> > > a pointer value to NULL when the pointer is leaving scope is almost as
> > > useful as using a hard hat to prevent drowning in a pool.
> > > 
> > > There is no way to use the variable which would not be a completely
> > > different class of bug. Also in no way does this keep the stack "tidy".
> > > All stacked variables must still be considered uninitialized even if
> > > we'd make sure that everything resets it's stack copies.
> > 
> > In "normal" execution setting the variable to NULL is indeed useless
> > but the benefit of defensive programming comes in the abnormal execution
> > scenarios. The stack region being cleared in the current function is the
> > same chunk of memory that will be used for stack in the next function
> > the caller invokes. Having the stack littered with data from variables
> > that are now out of scope is very useful to people exploiting security
> > bugs. Zero'ing out pointers and memory is a worthwhile thing todo even
> 
> I will not buy that this is a security thing as long as  VIR_AUTOFREE
> and VIR_FREE are a thing. It sounds great that we try to zero out the
> pointers but if we keep the values on the heap it does not buy that
> much.

VIR_FREE works the same way as this code does - it zeros out the
pointer to the pointer after free'ing it.

The VIR_DEFINE_AUTOPTR_FUNC macro is ultimately a hack to deal with
the fact that most other *Free APIs are just taking a pointer arg,
not a pointer to a pointer.

When we introduced that I requested that we actally change the other
APIs to take a pointer to a pointer, so that they would nullify the
the pointer after free'ing it, just like VIR_FREE does.

This VIR_DEFINE_AUTOPTR_FUNC does the nullify so that this works
the way we'd ultimately like all the *Free methods to work.

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