Re: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
On Tue, Feb 26, 2019 at 03:03:01PM +0100, Peter Krempa wrote: > On Tue, Feb 26, 2019 at 13:30:55 +, 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
On Tue, Feb 26, 2019 at 13:30:55 +, 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. Additionally we don't do this for any temp pointer which may point into the same memory. > 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. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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. The only thing that comes into my mind where this would "save" us is if you assign a pointer of a pointer defined by VIR_AUTOPTR to a variable which also has a __attribute(cleanup... type handler which would dereference it. That is also wrong on the basis that we should never rely on the order those cleanup functions are executed even if the standard does specify it. Please don't mix this up with what VIR_FREE is doing. Resetting pointers in VIR_FREE prevents bugs as the pointer is still a valid variable. > Originally, I wanted to argue, that this might introduce an issue in loops if > you pass the temporary variable to a function by reference and then exiting on > failure would free some garbage, but that's irrelevant since Sukrit introduced > a syntax check to ensure, VIR_AUTO variables are always initialized. > > > > > Making the inline function contain less code also decreases the > > possibility that for a given type the inlining will be declined by the > > compiler due to increasing code size. > > I honestly doubt that re-setting a variable to NULL will have any impact on > this and it's such a trivial operation that the compiler might optimize this > for you in some way. Well, I unfortunately can't seem to be able to reproduce this claim at this point. The compiled code does have the extra instruction which clears the variable. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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). Originally, I wanted to argue, that this might introduce an issue in loops if you pass the temporary variable to a function by reference and then exiting on failure would free some garbage, but that's irrelevant since Sukrit introduced a syntax check to ensure, VIR_AUTO variables are always initialized. > > Making the inline function contain less code also decreases the > possibility that for a given type the inlining will be declined by the > compiler due to increasing code size. I honestly doubt that re-setting a variable to NULL will have any impact on this and it's such a trivial operation that the compiler might optimize this for you in some way. I think this patch should be dropped from the series and left as is. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
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.
Making the inline function contain less code also decreases the
possibility that for a given type the inlining will be declined by the
compiler due to increasing code size.
Signed-off-by: Peter Krempa
---
src/util/viralloc.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 15451d4673..50a07d4fa3 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -612,7 +612,6 @@ void virAllocTestHook(void (*func)(int, void*), void *data);
{ \
if (*_ptr) \
(func)(*_ptr); \
-*_ptr = NULL; \
}
# define VIR_AUTOCLEAN_FUNC_NAME(type) type##AutoClean
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
