[Samba] Cannot set ACL rights for group "Authenticated Users"
(SID S-1-5-11)\ Two bugs in change svn-22481
Jeremy Allison
jra at samba.org
Mon Apr 30 16:13:48 GMT 2007
On Mon, Apr 30, 2007 at 06:15:01PM +0200, Jens Nissen wrote:
> Thanks Jeremy for the ACL-fix (svn-Revision 22481).
> It points out the way to go, even though I think, you had a bad day:
> IMHO, There are two bugs:
>
> (a) A minor bug in your util_sid.c - change.
> The additional test
>
> if (sid_equal(sid, &global_sid_System))
> return True;
>
> is superfluous, as the global_sid_System is part of NT-Authority which
> is lateron tested with
>
> if (sid_equal(&dom, &global_sid_NT_Authority))
> return True;
>
> I recommend reverting util_sic.c to revision 22480.
Ok, I'll check this out.
> (b) A severe bug in your change to posix_acls.c
>
> You have moved the test for non-mappable SIDs from a point BEFORE
> SMB_MALLOC_P to a point beyond the call "current_ace = SMB_MALLOC_P(---)".
>
> Thus your fix leaks memory of size "canon_ace" each time a non-mappable
> SID is called.
>
> The correct code in create_canon_ace_lists should look like this:
>
> /*
> * Silently ignore map failures in non-mappable SIDs (NT
> Authority, BUILTIN etc).
> */
>
> if (non_mappable_sid(&psa->trustee)) {
> DEBUG(10,("create_canon_ace_lists: ignoring non-mappable SID %s\n",
> sid_to_string(str, &psa->trustee) ));
> SAFE_FREE(current_ace);
> continue;
> }
>
>
> I hope, I didn't miss a point in my analysis.
Nope - looks good. What you missed is that there
are other areas in this loop that return without
freeing current_ace, so your fix is good but not
quite general enough :-).
I'll add the extra SAFE_FREE's needed :-).
Thanks !
Jeremy.
More information about the samba
mailing list