Re: Slowdown

[Christopher Faylor <cgf at redhat dot com>]

[redirecting to cygwin-developers from a private discussion]
On Thu, Jun 27, 2002 at 10:40:59PM -0400, Pierre A. Humblet wrote:
>At 05:11 PM 6/27/2002 -0400, Christopher Faylor wrote:
>>I've made some more modifications to the env_* stuff.  I gave up on
>>the static buffer method (which is what you're seeing below) and just
>>added some more member variables to cygheap_user.  This is what Corinna
>>suggested originally, I think.  I should have listened to her...
>>
>I had a look, but the cvs build fails for me tonight and I can't test.

Details?

>On one level I think your changes take care of the static problem.
>Two minor comments:
>They may return empty environment variables, which AFAIK Windows 
>doesn't do. 

AFAIK, it doesn't return an empty environment variable anymore.

>The env_ functions never return a real NULL so !p can be removed from 
>line 810 of environ.cc

It does return NULL in some situations:

  const char *
  cygheap_user::env_logsrv (const char *name, size_t namelen)
  {
    if (test_uid (plogsrv, name, namelen))
      return plogsrv;
    
    if (!domain () || strcasematch (winname (), "SYSTEM"))
      return NULL;
    .
    .
    .
  }

Although maybe this shouldn't be NULL.

Regardless, it is certainly possible for cmalloc to return NULL so we
should still handle that case even if it is indicative of a more serious
problem.

>On another level, I don't think they solve the "slow start" problem.
>The program will still lookup a logserver the first time it execs :(,
>but not every time as before :)

If it is not a cygwin app, then that is (was) true.  I just added back
the "use the environment" stuff.  So, it should now use the environment
wherever it can, resorting to not using the environment only if setuid'ed
or the environment doesn't have the needed info.

I still don't understand why there would be any slowdown in the case of
running something from a -X mounted directory, though.  It seems like
this should still have been faster than it appeared to be.

>IMHO this project has ballooned unexpectedly. The mission should be
>very simple: don't do any lookup and don't change the environment
>(beyond HOME, TERM and the form of TMP etc..) when entering from Windows, 
>until there is a setuid ( you have defined a nice user.issetuid () to 
>detect that event). Then recompute what's needed and stay put until the 
>next setuid. 

That was the goal of the changes.

Btw, couldn't issetuid just be:

  bool cygheap_user::issetuid () { return orig_uid != my->uid;}

?

This is what I just used in my recent changes to uinfo.cc.  It seems
like it is one less test and is a little clearer what is going on.
But I could be missing something.

>If on entering from Windows a user doesn't have HOME defined and
>doesn't have a home in passwd, and doesn't have HOMEDRIVE/HOMEPATH,
>then let's set HOME to some sane value such as WINDIR or c:\,
>rather than risking a lookup. The user will eventually run mkpasswd...

Really?  That's a departure from the way things used to work, isn't it?
Don't you think there will be complaints?

Corinna?

>>Btw, can we continue this discussion on cygwin-developers?  I'll subscribe
>>you there, if that's ok, Pierre.
>
>Sure, if you think it's useful, thanks a lot. I am still replying directly, 
>simply forward it.

Done.

cgf

>Pierre
>
>>On Thu, Jun 27, 2002 at 12:41:39PM -0400, Pierre A. Humblet wrote:
>>>Back in my usual work place. The cvs dll I build last nite crashes,
>>>so I investigated with the previous one. 
>>>I also looked at what Chris has done in cygheap_user::set_name,
>>>but I am not sure I understand it.
>>>
>>>Look at this:
>>>E:\bin>set HOMEDRIVE
>>>HOMEDRIVE=H:
>>>
>>>E:\bin>sh           <= first
>>>$ echo $HOMEDRIVE
>>>H:
>>>$ sh                <= second 
>>>$ echo $HOMEDRIVE
>>>H:
>>>$ sh                <= third
>>>$ echo $HOMEDRIVE
>>>
>>>$
>>>What's happening is that at on entry into the 2nd sh
>>>HOMEDRIVE is still set to H: in the environment but 
>>>user.homedrive is "". This propagates into the env of
>>>the 3rd sh.
>>>
>>>Looking into it with gdb, CYGWIN_SLEEP etc.. it appears 
>>>that the cygheap is not copied properly into the 2nd sh.
>>>
>>>   at ../../../../src/winsup/cygwin/cygheap.cc:121
>>>121       if (newaddr != cygheap)
>>>(gdb) n
>>>147       ForceCloseHandle1 (child_proc_info->cygheap_h, passed_cygheap_h);
>>>(gdb) p cygheap->user
>>>$8 = {pname = 0x61610118 "PHumbletU", plogsrv = 0x61613b50 "\\\\ADMIN1", 
>>>  pdomain = 0x61613b28 "ASTRALPOINT", homedrive = 0x61101108 "", 
>>>  homepath = 0x61100ff8 "", winname = 0x61613b00 "phumblet", 
>>>  psid = 0x61610248, orig_psid = 0x61610290, 
>>>  static homedrive_env_buf = "\000\000", 
>>>  static homepath_env_buf = '\000' <repeats 260 times>, 
>>>  static userprofile_env_buf = '\000' <repeats 260 times>, orig_uid =
>11054, 
>>>  orig_gid = 10513, real_uid = 11054, real_gid = 10513, token = 0xffffffff, 
>>>  impersonated = 0}
>>>
>>>Notice that homedrive and friends are not NULL (indicating no reset
>>>by set_name) but that homedrive_env_buf is all 0 (dunno why).
>>>It wasn't so just before CreateProcess in the first sh.
>>>(this is on NT).
>>>
>>>I noticed another issue:
>>>As discussed previously, uinfo initializes user.homedrive and homepath
>>>from the env, but not plogsrv and the others. As a result, the logserver
>>>will be looked up in build_env (as well as domain and winname).
>>>This will cause a slowdown if the server is unreachable.
>>>In addition userprofile_env_buf is filled every time by reading the
>>>registry (no caching) in cygheap_user::env_userprofile ()
>>>
>>>A safe solution should avoid ALL non-environment lookups (LookupAccountSid,
>>>NetUserGetInfo, get_logon_server, registry, even passwd ) in the normal 
>>>cases (no setuid). What I outlined yesterday does that.