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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
