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

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

if they're going out of scope. Stack corruption is common in C as it is a memory-unsafe language, so any steps we can take to provide a cleaner state to the stack is useful if it doesn't cost us performance. So I agree with Erik that we shouldn't remove these kind of assignments, even if they are "dead code" during normal execution codepaths.