"Unknown" Groups Not Caching - Debugging

"Unknown" Groups Not Caching - Debugging

[Bryan Berns]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

Posting here because that's what Corinna told me to do.

So I was looking into the issue I found with Cygwin not caching
orphaned / unknown groups.  At least for my test case, this appears to
be rooted in the RID getting striped from the end of the SID just
prior to the line being outputted (line 2464 from the exert below)
within fetch_account_from_windows().

uinfo.cc: 2460: /* Check if we know the domain.  If so, create a passwd/group
uinfo.cc: 2461:    entry with domain prefix and RID as username. */
uinfo.cc: 2462: PDS_DOMAIN_TRUSTSW td = NULL;
uinfo.cc: 2463:
uinfo.cc: 2464: sid_sub_auth_count (sid) = sid_sub_auth_count (sid) - 1;

It's not clear to me, in general, why we're interested in creating a
"passwd/group entry with domain prefix and RID as username".  There
are quite a few nuances and special cases in this function so I feel
like any attempt on my part to produce a patch on this will likely be
completely errant.  Might someone familiar with check weigh-in on
these findings thus far?

I've attached my test script (Windows Batch).

[-- Attachment #2: LocalGroupTest.txt --]
[-- Type: text/plain, Size: 1213 bytes --]

@ECHO OFF
NET LOCALGROUP LocalGroupTest /ADD
IF EXIST C:\LocalGroupTestDir RD /S /Q C:\LocalGroupTestDir 
MKDIR C:\LocalGroupTestDir
FOR /L %%X IN (1,1,1000) DO ECHO TestFile > C:\LocalGroupTestDir\Test.%%X
FOR /L %%X IN (1,1,1000) DO ICACLS C:\LocalGroupTestDir\Test.%%X /grant LocalGroupTest:M > NUL
ECHO Running Time Tests While Group Exists...
"C:\cygwin\bin\bash.exe" -c 'time /bin/ls -l /cygdrive/c/LocalGroupTestDir ^> /dev/null'
"C:\cygwin\bin\bash.exe" -c 'time /bin/ls -l /cygdrive/c/LocalGroupTestDir ^> /dev/null'
"C:\cygwin\bin\bash.exe" -c 'time /bin/ls -l /cygdrive/c/LocalGroupTestDir ^> /dev/null'
"C:\cygwin\bin\bash.exe" -c '/bin/strace -o run-good.txt /bin/ls -l /cygdrive/c/LocalGroupTestDir ^> /dev/null'
NET LOCALGROUP LocalGroupTest /DELETE
ECHO Running Time Tests After Group Deletion...
"C:\cygwin\bin\bash.exe" -c 'time /bin/ls -l /cygdrive/c/LocalGroupTestDir ^> /dev/null'
"C:\cygwin\bin\bash.exe" -c 'time /bin/ls -l /cygdrive/c/LocalGroupTestDir ^> /dev/null'
"C:\cygwin\bin\bash.exe" -c 'time /bin/ls -l /cygdrive/c/LocalGroupTestDir ^> /dev/null'
"C:\cygwin\bin\bash.exe" -c '/bin/strace -o run-bad.txt /bin/ls -l /cygdrive/c/LocalGroupTestDir ^> /dev/null'
PAUSE

Re: "Unknown" Groups Not Caching - Debugging

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2009 bytes --]

On Apr 16 22:50, Bryan Berns wrote:
> Posting here because that's what Corinna told me to do.
> 
> So I was looking into the issue I found with Cygwin not caching
> orphaned / unknown groups.  At least for my test case, this appears to
> be rooted in the RID getting striped from the end of the SID just
> prior to the line being outputted (line 2464 from the exert below)
> within fetch_account_from_windows().
> 
> uinfo.cc: 2460: /* Check if we know the domain.  If so, create a passwd/group
> uinfo.cc: 2461:    entry with domain prefix and RID as username. */
> uinfo.cc: 2462: PDS_DOMAIN_TRUSTSW td = NULL;
> uinfo.cc: 2463:
> uinfo.cc: 2464: sid_sub_auth_count (sid) = sid_sub_auth_count (sid) - 1;
> 
> It's not clear to me, in general, why we're interested in creating a
> "passwd/group entry with domain prefix and RID as username".

Counter question.  Why not?  The result is that you know it's some
account from a known domain, and the combination of the domain name
and the RID results in a unique account name.

Thanks for looking into this and tracking it down to this point.
The problem in your case is that the sid isn't reverted in the
following !domain branch.

Please try this patch:

diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index 6186327..91d1d1c 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -2475,10 +2475,10 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
 		  posix_offset = fetch_posix_offset (td, &loc_ldap);
 		  break;
 		}
+	  sid_sub_auth_count (sid) = sid_sub_auth_count (sid) + 1;
 	}
       if (domain)
 	{
-	  sid_sub_auth_count (sid) = sid_sub_auth_count (sid) + 1;
 	  wcscpy (dom, domain);
 	  __small_swprintf (name = namebuf, L"%W(%u)",
 			    is_group () ? L"Group" : L"User",


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: "Unknown" Groups Not Caching - Debugging

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

On Apr 17 10:23, Corinna Vinschen wrote:
> On Apr 16 22:50, Bryan Berns wrote:
> > Posting here because that's what Corinna told me to do.
> > 
> > So I was looking into the issue I found with Cygwin not caching
> > orphaned / unknown groups.  At least for my test case, this appears to
> > be rooted in the RID getting striped from the end of the SID just
> > prior to the line being outputted (line 2464 from the exert below)
> > within fetch_account_from_windows().
> > 
> > uinfo.cc: 2460: /* Check if we know the domain.  If so, create a passwd/group
> > uinfo.cc: 2461:    entry with domain prefix and RID as username. */
> > uinfo.cc: 2462: PDS_DOMAIN_TRUSTSW td = NULL;
> > uinfo.cc: 2463:
> > uinfo.cc: 2464: sid_sub_auth_count (sid) = sid_sub_auth_count (sid) - 1;
> > 
> > It's not clear to me, in general, why we're interested in creating a
> > "passwd/group entry with domain prefix and RID as username".
> 
> Counter question.  Why not?  The result is that you know it's some
> account from a known domain, and the combination of the domain name
> and the RID results in a unique account name.
> 
> Thanks for looking into this and tracking it down to this point.
> The problem in your case is that the sid isn't reverted in the
> following !domain branch.
> 
> Please try this patch:
> 
> diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> index 6186327..91d1d1c 100644
> --- a/winsup/cygwin/uinfo.cc
> +++ b/winsup/cygwin/uinfo.cc
> @@ -2475,10 +2475,10 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
>  		  posix_offset = fetch_posix_offset (td, &loc_ldap);
>  		  break;
>  		}
> +	  sid_sub_auth_count (sid) = sid_sub_auth_count (sid) + 1;
>  	}
>        if (domain)
>  	{
> -	  sid_sub_auth_count (sid) = sid_sub_auth_count (sid) + 1;
>  	  wcscpy (dom, domain);
>  	  __small_swprintf (name = namebuf, L"%W(%u)",
>  			    is_group () ? L"Group" : L"User",
> 

I applied this patch to git HEAD.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: "Unknown" Groups Not Caching - Debugging

[Bryan Berns]

On Fri, Apr 17, 2015 at 4:23 AM, Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> On Apr 16 22:50, Bryan Berns wrote:
>>
>> So I was looking into the issue I found with Cygwin not caching
>> orphaned / unknown groups.  At least for my test case, this appears to
>> be rooted in the RID getting striped from the end of the SID just
>> prior to the line being outputted (line 2464 from the exert below)
>> within fetch_account_from_windows().
>>
>> uinfo.cc: 2460: /* Check if we know the domain.  If so, create a passwd/group
>> uinfo.cc: 2461:    entry with domain prefix and RID as username. */
>> uinfo.cc: 2462: PDS_DOMAIN_TRUSTSW td = NULL;
>> uinfo.cc: 2463:
>> uinfo.cc: 2464: sid_sub_auth_count (sid) = sid_sub_auth_count (sid) - 1;
>>
>> It's not clear to me, in general, why we're interested in creating a
>> "passwd/group entry with domain prefix and RID as username".
>
> Counter question.  Why not?  The result is that you know it's some
> account from a known domain, and the combination of the domain name
> and the RID results in a unique account name.
>
> Thanks for looking into this and tracking it down to this point.
> The problem in your case is that the sid isn't reverted in the
> following !domain branch.
>
> Please try this patch:
>
> diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> index 6186327..91d1d1c 100644
> --- a/winsup/cygwin/uinfo.cc
> +++ b/winsup/cygwin/uinfo.cc
> @@ -2475,10 +2475,10 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
>                   posix_offset = fetch_posix_offset (td, &loc_ldap);
>                   break;
>                 }
> +         sid_sub_auth_count (sid) = sid_sub_auth_count (sid) + 1;
>         }
>        if (domain)
>         {
> -         sid_sub_auth_count (sid) = sid_sub_auth_count (sid) + 1;
>           wcscpy (dom, domain);
>           __small_swprintf (name = namebuf, L"%W(%u)",
>                             is_group () ? L"Group" : L"User",

Yeah, works like a charm -- I guess I have to have more confidence
since that's actually what I did to test it works when I discovered
the issue.  I guess my earlier question rooted in the thought that the
original author wanted to explicitly restore the SID if and only if a
domain was defined.  Was this a simple misplacement of the code to
required restore the RID to the SID or is there some forgotten nuanced
scenario that we're not thinking of where we actually want to store
the SID with the RID removed?  I can't think of one, but then again
this is my first time looking a this code.

Thanks!

Re: "Unknown" Groups Not Caching - Debugging

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

On Apr 17 06:21, Bryan Berns wrote:
> On Fri, Apr 17, 2015 at 4:23 AM, Corinna Vinschen
> <corinna-cygwin@cygwin.com> wrote:
> > On Apr 16 22:50, Bryan Berns wrote:
> >>
> >> So I was looking into the issue I found with Cygwin not caching
> >> orphaned / unknown groups.  At least for my test case, this appears to
> >> be rooted in the RID getting striped from the end of the SID just
> >> prior to the line being outputted (line 2464 from the exert below)
> >> within fetch_account_from_windows().
> >>
> >> uinfo.cc: 2460: /* Check if we know the domain.  If so, create a passwd/group
> >> uinfo.cc: 2461:    entry with domain prefix and RID as username. */
> >> uinfo.cc: 2462: PDS_DOMAIN_TRUSTSW td = NULL;
> >> uinfo.cc: 2463:
> >> uinfo.cc: 2464: sid_sub_auth_count (sid) = sid_sub_auth_count (sid) - 1;
> >>
> >> It's not clear to me, in general, why we're interested in creating a
> >> "passwd/group entry with domain prefix and RID as username".
> >
> > Counter question.  Why not?  The result is that you know it's some
> > account from a known domain, and the combination of the domain name
> > and the RID results in a unique account name.
> >
> > Thanks for looking into this and tracking it down to this point.
> > The problem in your case is that the sid isn't reverted in the
> > following !domain branch.
> >
> > Please try this patch:
> >
> > diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> > index 6186327..91d1d1c 100644
> > --- a/winsup/cygwin/uinfo.cc
> > +++ b/winsup/cygwin/uinfo.cc
> > @@ -2475,10 +2475,10 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
> >                   posix_offset = fetch_posix_offset (td, &loc_ldap);
> >                   break;
> >                 }
> > +         sid_sub_auth_count (sid) = sid_sub_auth_count (sid) + 1;
> >         }
> >        if (domain)
> >         {
> > -         sid_sub_auth_count (sid) = sid_sub_auth_count (sid) + 1;
> >           wcscpy (dom, domain);
> >           __small_swprintf (name = namebuf, L"%W(%u)",
> >                             is_group () ? L"Group" : L"User",
> 
> Yeah, works like a charm

Cool, thanks for testing.

> -- I guess I have to have more confidence
> since that's actually what I did to test it works when I discovered
> the issue.  I guess my earlier question rooted in the thought that the
> original author wanted to explicitly restore the SID if and only if a
> domain was defined.  Was this a simple misplacement of the code to
> required restore the RID to the SID or is there some forgotten nuanced
> scenario that we're not thinking of where we actually want to store
> the SID with the RID removed?  I can't think of one, but then again
> this is my first time looking a this code.

Well, I assume it's a result of a misplacement which I never realized
because of testing with domain accounts only :}


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]