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

Andrew Bartlett abartlet at samba.org
Wed Oct 31 17:09:14 MDT 2012


On Wed, 2012-10-31 at 16:02 -0700, Jeremy Allison wrote:
> 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.

Indeed!  I got tripped up by the make_sec_acl() on the next line, which
duplicates the memory. 

Thanks for your close attention to this!

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vfs_acl_common-In-add_directory_inheritable_componen.patch
Type: text/x-patch
Size: 1503 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20121101/ef7bedca/attachment.bin>


More information about the samba-technical mailing list