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

[Andrew Bartlett]

On Fri, 2012-11-02 at 08:50 +0100, Christian Ambach wrote:
> Hi Andrew,
> 
> On 11/01/2012 01:05 AM, Andrew Bartlett wrote:
> 
> > 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.
> 
> The testcase was pretty simple: open the share in Windows Explorer (this
> triggers the se_access_check valgrind reports) and then try to display
> the ACL via Properties->Security Tab. This one failed to display anything.

I was referring to the python based tests I've been writing to unit test
this area (samba.tests.posixacl).  I'm keen to extend that to fully
cover the important codepaths here. 

> > Indeed.  I think this (improved) patch handles that case properly,
> > getting new_ace_list under psd->dacl, where it needs to be in the tree,
> > or allocating it with talloc_tos() when it will be duplicated anyway.
> 
> Yes, looks much better with this patch now.
> The stackframe is empty (besides two small strings), no valgrind errors
> and Windows Explorer is happy.

Great!

> I will now continue to work on the GPFS part of it (already found some
> memory management issues in the existing POSIX ACL code).
> 
> You can have a look at the fix-acls2 branch in my git repo:
> http://gitweb.samba.org/?p=ambi/samba.git;a=shortlog;h=refs/heads/fix-acls2

This looks good, and I do really, really appriciate your dilligent
attention to this area. 

However, I do notice one issue.  You can't do this:

+               DATA_BLOB aclblob;
+
+               aclblob.data = (uint8_t*) pacl;
+               aclblob.length = talloc_get_size(pacl);

https://gitweb.samba.org/?p=ambi/samba.git;a=commitdiff;h=7d80dfd6f7602dddd43c14566ee43fed06b0ad31

If you want to use the NFSv4 layer as the linearisation, then we need to
re-define these structures as IDL (like I did with the posix ACL) and
then linearlise the in-memory structure.  What you have here, as I read
the code, will just get the head object, including any padding, not the
actual ACL. 

That said, you previously mentioned an opaque blob 'backup software'
interface, and perhaps I've misunderstood your intent and you were just
applying a mock-up until you got that coded.

Thanks,

Andrew Bartlett

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