[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