[PATCH] Fix the talloc tree for returned SD (was Re: [PATCH][WIP] Make vfs_acl_xattr use hash of the posix ACL)

[Jeremy Allison]

On Thu, Nov 01, 2012 at 09:56:00AM +1100, Andrew Bartlett wrote:
> On Wed, 2012-10-31 at 22:56 +0100, Christian Ambach wrote:
> > On 10/31/2012 02:39 PM, Christian Ambach wrote:
> > 
> > >> However, it's not in the posix_acls or vfs_acl_common code as far as I
> > >> can tell, because valgrind over that code (now quite well exercised by
> > >> samba.tests.posixacl) is clean.
> > 
> > In case it'll help, I added some talloc_report statements to
> > get_nt_acl_internal before it frees the stackframe. See the attachment
> > for details (did not want the list to arbitrarly break lines).
> > 
> > --- a/source3/modules/vfs_acl_common.c
> > +++ b/source3/modules/vfs_acl_common.c
> > @@ -670,6 +670,8 @@ static NTSTATUS 
> > get_nt_acl_internal(vfs_handle_struct *handle,
> >          /* The VFS API is that the ACL is expected to be on mem_ctx */
> >          *ppdesc = talloc_move(mem_ctx, &psd);
> > 
> > +       talloc_report_full(frame, stderr);
> > +       talloc_report_full(mem_ctx, stderr);
> >          TALLOC_FREE(frame);
> >          return NT_STATUS_OK;
> >   }
> > 
> 
> I think this is the fix.
> 
> It doesn't get covered by my tests, because so far they don't cover
> directories (something I need to fix anyway), so if you can verify it in
> your known failing case, it would be most appreciated.
> 
> (also attached is what I was using to try and eliminate the error to
> either in posix_acls.c or vfs_acl_common.c, in case you were curious,
> but in the end it was old-fashioned manual inspection that found it)
> 
> Thanks!
> 
> Andrew Bartlett
> 
> -- 
> Andrew Bartlett                                http://samba.org/~abartlet/
> Authentication Developer, Samba Team           http://samba.org
> 

> >From 9e8341153681b3224a1218c2d3a3010fc2c06410 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Thu, 1 Nov 2012 09:51:28 +1100
> Subject: [PATCH] vfs_acl_common: In add_directory_inheritable_components
>  allocate on psd as parent
> 
> When we add a new DACL to the security descriptor, we need to use the SD as the memory
> context, so we can talloc_move() it as a tree to a new parent.
> 
> Andrew Bartlett
> ---
>  source3/modules/vfs_acl_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 959ec23..b005239 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -330,7 +330,7 @@ static NTSTATUS add_directory_inheritable_components(vfs_handle_struct *handle,
>  		psd->dacl->aces = new_ace_list;
>  		psd->dacl->num_aces += 3;
>  	} else {
> -		psd->dacl = make_sec_acl(talloc_tos(),
> +		psd->dacl = make_sec_acl(psd,
>  				NT4_ACL_REVISION,
>  				3,
>  				new_ace_list);
> -- 
> 1.7.11.7

Andrew,

	There's also the case with new_ace_list being allocated
on talloc_tos() also.

198         struct security_ace *new_ace_list = talloc_zero_array(talloc_tos(),
199                                                 struct security_ace,
200                                                 num_aces + 3);

This gets used in:

255         if (psd->dacl) {
256                 psd->dacl->aces = new_ace_list;
257                 psd->dacl->num_aces += 3;
258         } else {

so both places need to be fixed.

Jeremy.