[PATCH][WIP] Make vfs_acl_xattr use hash of the posix ACL

[Jeremy Allison]

On Tue, Oct 30, 2012 at 02:52:18PM +0100, Christian Ambach wrote:
> Hi Andrew,
> 
> On 10/25/2012 11:52 AM, Andrew Bartlett wrote:
> 
> >In the meantime, I've finally written the tests I was interested in.
> >These now show that the NT ACL invalidation code works correctly, as
> >well as testing the mapping of posix ACLs that have never had an NT ACL
> >set.
> 
> I have tried to fit vfs_gpfs to the helper functions and test the result.
> 
> You can find the patches in my ambi/samba.git repo (fix-acls2 branch).
> 
> But I was stopped pretty early by spurious smbd aborts and corrupted
> ACLs (especially the SIDs) on the wire that even occur when using a
> local ext3 + acl_xattr.
> 
> valgrind reports lots of errors like these:
> 
> ==25355== Invalid read of size 4
> ==25355==    at 0x5C8E83: dom_sid_compare (dom_sid.c:73)
> ==25355==    by 0x5C8ED8: dom_sid_equal (dom_sid.c:85)
> ==25355==    by 0x5CA7D8: security_token_has_sid (security_token.c:110)
> ==25355==    by 0x5ADBCC: se_access_check (access_check.c:229)
> ==25355==    by 0x5AE0A2: se_file_access_check (access_check.c:307)
> ==25355==    by 0x28643A: smbd_check_access_rights (open.c:137)
> ==25355==    by 0x234E6F: dptr_create (dir.c:534)
> ==25355==    by 0x276CB0: call_trans2findfirst (trans2.c:2499)
> ==25355==    by 0x27BA20: handle_trans2 (trans2.c:8583)
> ==25355==    by 0x27D5EF: reply_trans2 (trans2.c:8869)
> ==25355==    by 0x2A7ED2: switch_message (process.c:1551)
> ==25355==    by 0x2A995C: process_smb (process.c:1587)
> ==25355==  Address 0xf3e197c is 156 bytes inside a block of size 792 free'd
> ==25355==    at 0x4C23D72: free (vg_replace_malloc.c:325)
> ==25355==    by 0x84D5ED: _talloc_free_internal (talloc.c:942)
> ==25355==    by 0x84B522: _talloc_free (talloc.c:1355)
> ==25355==    by 0x108AFE94: get_nt_acl_internal (vfs_acl_common.c:673)
> ==25355==    by 0x108B0EC5: get_nt_acl_common (vfs_acl_common.c:701)
> ==25355==    by 0x2918FC: smb_vfs_call_get_nt_acl (vfs.c:2183)
> ==25355==    by 0x28640C: smbd_check_access_rights (open.c:115)
> ==25355==    by 0x234E6F: dptr_create (dir.c:534)
> ==25355==    by 0x276CB0: call_trans2findfirst (trans2.c:2499)
> ==25355==    by 0x27BA20: handle_trans2 (trans2.c:8583)
> ==25355==    by 0x27D5EF: reply_trans2 (trans2.c:8869)
> ==25355==    by 0x2A7ED2: switch_message (process.c:1551)
> 
> 
> I suspect that your introduction of a stackframe in get_nt_acl_common()
> now reveals that the POSIX ACL code has a weird memory hierarchy and
> also relies on talloc_tos() pointing to the same stackframe as for the
> upper callers in smbd main code.
> 
> To me it seems as if posix_get_nt_acl_common() (and all the functions
> that it calls) do not make sure that all the data returned as **ppdesc
> are hanging off the same talloc context. Some seem to remain allocated
> on talloc_tos() and so they are freed before it gets passed back to the
> smbd code (as get_nt_acl_internal finally frames its own stackframe),
> leading to corrupted memory.
> 
> Once this has been resolved, I can continue testing the GPFS integration
> code.

Oh that's annoying. This is what I was worried about with the introduction
of the extraneous stackframes into this code.

The whole thing was written to expect that the talloc_tos() would be
the one created from the incoming client request - this is convention
inside smbd.

Sub-stackframes should only be created in very special circumstances
when you know you'll be needing to allocate and free temporary memory
that isn't needed higher up in the callers request frame.

Jeremy.