[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