public inbox for cygwin-patches@cygwin.com
 help / color / mirror / Atom feed
* [PATCH] Allow overriding the home directory via the HOME variable
@ 2015-09-16 13:06 Johannes Schindelin
  2015-10-21 18:32 ` Corinna Vinschen
  2015-12-17 18:05 ` [PATCH v2 0/2] Support deriving the current user's home directory via HOME Johannes Schindelin
  0 siblings, 2 replies; 25+ messages in thread
From: Johannes Schindelin @ 2015-09-16 13:06 UTC (permalink / raw)
  To: cygwin-patches

	* uinfo.cc (cygheap_pwdgrp::get_home): Offer an option in
	nsswitch.conf that let's the environment variable HOME (or
	HOMEDRIVE & HOMEPATH, or USERPROFILE) define the home
	directory.

	* ntsec.xml: Document the `env` schema.

Detailed comments:

In the context of Git for Windows, it is a well-established technique
to let the `$HOME` variable define where the current user's home
directory is, falling back to `$HOMEDRIVE$HOMEPATH` and `$USERPROFILE`.

The idea is that we want to share user-specific settings between
programs, whether they be Cygwin, MSys2 or not.  Unfortunately, we
cannot blindly activate the "db_home: windows" setting because in some
setups, the user's home directory is set to a hidden directory via an
UNC path (\\share\some\hidden\folder$) -- something many programs
cannot handle correctly.

The established technique is to allow setting the user's home directory
via the environment variables mentioned above.  This has the additional
advantage that it is much faster than querying the Windows user database.

Of course this scheme needs to be opt-in.  For that reason, it needs
to be activated explicitly via `db_home: env` in `/etc/nsswitch.conf`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 winsup/cygwin/cygheap.h |  3 ++-
 winsup/cygwin/uinfo.cc  | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 winsup/doc/ntsec.xml    | 20 ++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h
index fd84814..097d50f 100644
--- a/winsup/cygwin/cygheap.h
+++ b/winsup/cygwin/cygheap.h
@@ -414,7 +414,8 @@ public:
     NSS_SCHEME_UNIX,
     NSS_SCHEME_DESC,
     NSS_SCHEME_PATH,
-    NSS_SCHEME_FREEATTR
+    NSS_SCHEME_FREEATTR,
+    NSS_SCHEME_ENV
   };
   struct nss_scheme_t {
     nss_scheme_method	method;
diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index da5809f..c74954a 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -780,6 +780,8 @@ cygheap_pwdgrp::nss_init_line (const char *line)
 		    scheme[idx].method = NSS_SCHEME_UNIX;
 		  else if (NSS_CMP ("desc"))
 		    scheme[idx].method = NSS_SCHEME_DESC;
+		  else if (NSS_CMP ("env"))
+		    scheme[idx].method = NSS_SCHEME_ENV;
 		  else if (NSS_NCMP ("/"))
 		    {
 		      const char *e = c + strcspn (c, " \t");
@@ -970,6 +972,40 @@ fetch_from_path (cyg_ldap *pldap, PUSER_INFO_3 ui, cygpsid &sid, PCWSTR str,
   return ret;
 }
 
+static size_t
+fetch_env(LPCWSTR key, char *buf, size_t size)
+{
+  WCHAR wbuf[32767];
+  DWORD max = sizeof wbuf / sizeof *wbuf;
+  DWORD len = GetEnvironmentVariableW (key, wbuf, max);
+
+  if (!len || len >= max)
+    return 0;
+
+  len = sys_wcstombs (buf, size, wbuf);
+  return len && len < size ? len : 0;
+}
+
+static char *
+fetch_home_env (void)
+{
+  char home[32767];
+  size_t max = sizeof home / sizeof *home, len;
+
+  if (fetch_env (L"HOME", home, max)
+      || ((len = fetch_env (L"HOMEDRIVE", home, max))
+        && fetch_env (L"HOMEPATH", home + len, max - len))
+      || fetch_env (L"USERPROFILE", home, max))
+    {
+      tmp_pathbuf tp;
+      cygwin_conv_path (CCP_WIN_A_TO_POSIX | CCP_ABSOLUTE,
+	  home, tp.c_get(), NT_MAX_PATH);
+      return strdup(tp.c_get());
+    }
+
+  return NULL;
+}
+
 char *
 cygheap_pwdgrp::get_home (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 			  PCWSTR dnsdomain, PCWSTR name, bool full_qualified)
@@ -1029,6 +1065,9 @@ cygheap_pwdgrp::get_home (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 		}
 	    }
 	  break;
+	case NSS_SCHEME_ENV:
+	  home = fetch_home_env ();
+	  break;
 	}
     }
   return home;
@@ -1060,6 +1099,9 @@ cygheap_pwdgrp::get_home (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	  home = fetch_from_path (NULL, ui, sid, home_scheme[idx].attrib,
 				  dom, NULL, name, full_qualified);
 	  break;
+	case NSS_SCHEME_ENV:
+	  home = fetch_home_env ();
+	  break;
 	}
     }
   return home;
@@ -1079,6 +1121,7 @@ cygheap_pwdgrp::get_shell (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_FALLBACK:
 	  return NULL;
 	case NSS_SCHEME_WINDOWS:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_CYGWIN:
 	  if (pldap->fetch_ad_account (sid, false, dnsdomain))
@@ -1143,6 +1186,7 @@ cygheap_pwdgrp::get_shell (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_CYGWIN:
 	case NSS_SCHEME_UNIX:
 	case NSS_SCHEME_FREEATTR:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_DESC:
 	  shell = fetch_from_description (ui->usri3_comment, L"shell=\"", 7);
@@ -1223,6 +1267,8 @@ cygheap_pwdgrp::get_gecos (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 		sys_wcstombs_alloc (&gecos, HEAP_NOTHEAP, val);
 	    }
 	  break;
+	case NSS_SCHEME_ENV:
+	  break;
 	}
     }
   if (gecos)
@@ -1249,6 +1295,7 @@ cygheap_pwdgrp::get_gecos (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_CYGWIN:
 	case NSS_SCHEME_UNIX:
 	case NSS_SCHEME_FREEATTR:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_DESC:
 	  gecos = fetch_from_description (ui->usri3_comment, L"gecos=\"", 7);
diff --git a/winsup/doc/ntsec.xml b/winsup/doc/ntsec.xml
index ae0a119..9cee58c 100644
--- a/winsup/doc/ntsec.xml
+++ b/winsup/doc/ntsec.xml
@@ -1354,6 +1354,16 @@ schemata are the following:
 	      See <xref linkend="ntsec-mapping-nsswitch-desc"></xref>
 	      for a more detailed description.</listitem>
   </varlistentry>
+  <varlistentry>
+    <term><literal>env</literal></term>
+    <listitem>Derives the home directory from the environment variable
+	      <literal>HOME</literal> (falling back to
+	      <literal>HOMEDRIVE\HOMEPATH</literal> and
+	      <literal>USERPROFILE</literal>, in that order).  This is faster
+	      than the <term><literal>windows</literal></term> schema at the
+	      expense of determining only the current user's home directory
+	      correctly.</listitem>
+  </varlistentry>
 </variablelist>
 
 <para>
@@ -1487,6 +1497,16 @@ of each schema when used with <literal>db_home:</literal>
 	      for a detailed description.</listitem>
   </varlistentry>
   <varlistentry>
+    <term><literal>env</literal></term>
+    <listitem>Derives the home directory from the environment variable
+	      <literal>HOME</literal> (falling back to
+	      <literal>HOMEDRIVE\HOMEPATH</literal> and
+	      <literal>USERPROFILE</literal>, in that order).  This is faster
+	      than the <term><literal>windows</literal></term> schema at the
+	      expense of determining only the current user's home directory
+	      correctly.</listitem>
+  </varlistentry>
+  <varlistentry>
     <term><literal>@ad_attribute</literal></term>
     <listitem>AD only: The user's home directory is set to the path given
 	      in the <literal>ad_attribute</literal> attribute.  The path
-- 
2.5.2.windows.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Allow overriding the home directory via the HOME variable
  2015-09-16 13:06 [PATCH] Allow overriding the home directory via the HOME variable Johannes Schindelin
@ 2015-10-21 18:32 ` Corinna Vinschen
  2015-10-22 15:38   ` Johannes Schindelin
  2015-12-17 18:05 ` [PATCH v2 0/2] Support deriving the current user's home directory via HOME Johannes Schindelin
  1 sibling, 1 reply; 25+ messages in thread
From: Corinna Vinschen @ 2015-10-21 18:32 UTC (permalink / raw)
  To: cygwin-patches

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

On Sep 16 15:06, Johannes Schindelin wrote:
> 	* uinfo.cc (cygheap_pwdgrp::get_home): Offer an option in
> 	nsswitch.conf that let's the environment variable HOME (or
> 	HOMEDRIVE & HOMEPATH, or USERPROFILE) define the home
> 	directory.
> 
> 	* ntsec.xml: Document the `env` schema.
> 
> Detailed comments:
> 
> In the context of Git for Windows, it is a well-established technique
> to let the `$HOME` variable define where the current user's home
> directory is, falling back to `$HOMEDRIVE$HOMEPATH` and `$USERPROFILE`.
> 
> The idea is that we want to share user-specific settings between
> programs, whether they be Cygwin, MSys2 or not.  Unfortunately, we
> cannot blindly activate the "db_home: windows" setting because in some
> setups, the user's home directory is set to a hidden directory via an
> UNC path (\\share\some\hidden\folder$) -- something many programs
> cannot handle correctly.

-v, please.  Which applications can't handle that?  Why do we have to
care?

> The established technique is to allow setting the user's home directory
> via the environment variables mentioned above.  This has the additional
> advantage that it is much faster than querying the Windows user database.

But it's wrong.  We discussed this a couple of times on the Cygwin ML.
The underlying functionality generically implements the passwd entries.
Your "env" setting will return the same $HOME setting in the pw_dir
field for every user account.  All user accounts will have the same home
dir as your current user.  And the value is unreliable, too.  If another
user logs in, all accounts will have another $HOME, the one from the now
logged in user.  This is so wrong and potentially dangerous that I don't
think this belongs into Cygwin, sorry.


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 --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Allow overriding the home directory via the HOME variable
  2015-10-21 18:32 ` Corinna Vinschen
@ 2015-10-22 15:38   ` Johannes Schindelin
  2015-10-23  9:10     ` Corinna Vinschen
  0 siblings, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2015-10-22 15:38 UTC (permalink / raw)
  To: Corinna Vinschen; +Cc: cygwin-patches

Hi Corinna,

On Wed, 21 Oct 2015, Corinna Vinschen wrote:

> On Sep 16 15:06, Johannes Schindelin wrote:
> > 	* uinfo.cc (cygheap_pwdgrp::get_home): Offer an option in
> > 	nsswitch.conf that let's the environment variable HOME (or
> > 	HOMEDRIVE & HOMEPATH, or USERPROFILE) define the home
> > 	directory.
> > 
> > 	* ntsec.xml: Document the `env` schema.
> > 
> > Detailed comments:
> > 
> > In the context of Git for Windows, it is a well-established technique
> > to let the `$HOME` variable define where the current user's home
> > directory is, falling back to `$HOMEDRIVE$HOMEPATH` and `$USERPROFILE`.
> > 
> > The idea is that we want to share user-specific settings between
> > programs, whether they be Cygwin, MSys2 or not.  Unfortunately, we
> > cannot blindly activate the "db_home: windows" setting because in some
> > setups, the user's home directory is set to a hidden directory via an
> > UNC path (\\share\some\hidden\folder$) -- something many programs
> > cannot handle correctly.
> 
> -v, please.  Which applications can't handle that?  Why do we have to
> care?

Oh, the first one that comes to mind is `cmd.exe`. You cannot start
`cmd.exe` with a UNC working directory without getting complaints.

> > The established technique is to allow setting the user's home directory
> > via the environment variables mentioned above.  This has the additional
> > advantage that it is much faster than querying the Windows user database.
> 
> But it's wrong.  We discussed this a couple of times on the Cygwin ML.
> The underlying functionality generically implements the passwd entries.
> Your "env" setting will return the same $HOME setting in the pw_dir
> field for every user account.

No, it will not, because most users are not administrators. So they can
only set environment variables for themselves.

In most cases, `HOME` will not even be defined, but `HOMEDRIVE` and
`HOMEPATH` will, and they will be correct.

> All user accounts will have the same home dir as your current user.  And
> the value is unreliable, too.  If another user logs in, all accounts
> will have another $HOME, the one from the now logged in user.  This is
> so wrong and potentially dangerous that I don't think this belongs into
> Cygwin, sorry.

I ask you to reconsider. I am not trying to make this the default. And I
*need* a way to heed the `HOME` variable set by the user. For
backwards-compatibility with Git for Windows 1.x, where users frequently
adjusted the `HOME` variable to fix problems with MSys not handling their
home directory properly.

The patch itself is so simple that it cannot possibly cause a maintenance
burden, and by offering this feature as opt-in, users who are in need of
that feature can easily use it -- in Cygwin, MSys2, and yes, in Git for
Windows.

It would bring a major benefit to us.

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Allow overriding the home directory via the HOME variable
  2015-10-22 15:38   ` Johannes Schindelin
@ 2015-10-23  9:10     ` Corinna Vinschen
  2015-10-23  9:41       ` Corinna Vinschen
  0 siblings, 1 reply; 25+ messages in thread
From: Corinna Vinschen @ 2015-10-23  9:10 UTC (permalink / raw)
  To: cygwin-patches

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

On Oct 22 17:38, Johannes Schindelin wrote:
> Hi Corinna,
> 
> On Wed, 21 Oct 2015, Corinna Vinschen wrote:
> 
> > On Sep 16 15:06, Johannes Schindelin wrote:
> > > 	* uinfo.cc (cygheap_pwdgrp::get_home): Offer an option in
> > > 	nsswitch.conf that let's the environment variable HOME (or
> > > 	HOMEDRIVE & HOMEPATH, or USERPROFILE) define the home
> > > 	directory.
> > > 
> > > 	* ntsec.xml: Document the `env` schema.
> > > 
> > > Detailed comments:
> > > 
> > > In the context of Git for Windows, it is a well-established technique
> > > to let the `$HOME` variable define where the current user's home
> > > directory is, falling back to `$HOMEDRIVE$HOMEPATH` and `$USERPROFILE`.
> > > 
> > > The idea is that we want to share user-specific settings between
> > > programs, whether they be Cygwin, MSys2 or not.  Unfortunately, we
> > > cannot blindly activate the "db_home: windows" setting because in some
> > > setups, the user's home directory is set to a hidden directory via an
> > > UNC path (\\share\some\hidden\folder$) -- something many programs
> > > cannot handle correctly.
> > 
> > -v, please.  Which applications can't handle that?  Why do we have to
> > care?
> 
> Oh, the first one that comes to mind is `cmd.exe`. You cannot start
> `cmd.exe` with a UNC working directory without getting complaints.

Sure, but then again, why do we have to care?  Didn't you say GfW is
using bash?

> > > The established technique is to allow setting the user's home directory
> > > via the environment variables mentioned above.  This has the additional
> > > advantage that it is much faster than querying the Windows user database.
> > 
> > But it's wrong.  We discussed this a couple of times on the Cygwin ML.
> > The underlying functionality generically implements the passwd entries.
> > Your "env" setting will return the same $HOME setting in the pw_dir
> > field for every user account.
> 
> No, it will not, because most users are not administrators. So they can
> only set environment variables for themselves.

You're looking at the problem from the wrong direction.  Consider how
the mechanism works.  The setting in /etc/nsswitch.conf configures how
the passwd/group entries for all accounts are created.  Your "env"
setting fetches the environment of the *current* user and generates
the passwd entry for *all* user accounts from it.  Here's what happens:

  $ echo $USER
  corinna
  $ echo $HOME
  /home/corinna
  $ getent passwd $USER
  corinna:*:1049577:1049701:U-VINSCHEN\corinna,S-1-5-21-913048732-1697188782-3448811101-1001:/home/corinna:/bin/tcsh
  $ getent passwd Guest
  Guest:*:1049077:1049090:U-VINSCHEN\Guest,S-1-5-21-2913048732-1697188782-3448811101-501:/home/corinna:/bin/bash
  $ getent passwd Administrator
  Administrator:*:1049076:1049089:U-VINSCHEN\Administrator,S-1-5-21-2913048732-1697188782-3448811101-500:/home/corinna:/bin/bash
  [...]

This is plain wrong.  You can't seriously provide a mechanism which
fetches data from the current user to generate the output for all users
from there.

> In most cases, `HOME` will not even be defined, but `HOMEDRIVE` and
> `HOMEPATH` will, and they will be correct.

That's not the point.

> > All user accounts will have the same home dir as your current user.  And
> > the value is unreliable, too.  If another user logs in, all accounts
> > will have another $HOME, the one from the now logged in user.  This is
> > so wrong and potentially dangerous that I don't think this belongs into
> > Cygwin, sorry.
> 
> I ask you to reconsider.

I did, but the result is the same.  As is, the patch is not acceptable.

Here's what you *could* do:  Create a patch which fetches the pw_home
info from the current user environment for the current user only.  For
all other accounts, the info must be taken from elsewhere, one of the
other methods.  E.g., the env setting only affects the current user
passwd entry, not any other.  For all other accounts, the env method
provides no result, thus falling back to the next method in the row.


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 --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Allow overriding the home directory via the HOME variable
  2015-10-23  9:10     ` Corinna Vinschen
@ 2015-10-23  9:41       ` Corinna Vinschen
  2015-10-23 12:00         ` Johannes Schindelin
  0 siblings, 1 reply; 25+ messages in thread
From: Corinna Vinschen @ 2015-10-23  9:41 UTC (permalink / raw)
  To: cygwin-patches

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

On Oct 23 11:10, Corinna Vinschen wrote:
> On Oct 22 17:38, Johannes Schindelin wrote:
> > Hi Corinna,
> > 
> > On Wed, 21 Oct 2015, Corinna Vinschen wrote:
> > 
> > > On Sep 16 15:06, Johannes Schindelin wrote:
> > > > 	* uinfo.cc (cygheap_pwdgrp::get_home): Offer an option in
> > > > 	nsswitch.conf that let's the environment variable HOME (or
> > > > 	HOMEDRIVE & HOMEPATH, or USERPROFILE) define the home
> > > > 	directory.
> > > > 
> > > > 	* ntsec.xml: Document the `env` schema.
> > > > 
> > > > Detailed comments:
> > > > 
> > > > In the context of Git for Windows, it is a well-established technique
> > > > to let the `$HOME` variable define where the current user's home
> > > > directory is, falling back to `$HOMEDRIVE$HOMEPATH` and `$USERPROFILE`.
> > > > 
> > > > The idea is that we want to share user-specific settings between
> > > > programs, whether they be Cygwin, MSys2 or not.  Unfortunately, we
> > > > cannot blindly activate the "db_home: windows" setting because in some
> > > > setups, the user's home directory is set to a hidden directory via an
> > > > UNC path (\\share\some\hidden\folder$) -- something many programs
> > > > cannot handle correctly.
> > > 
> > > -v, please.  Which applications can't handle that?  Why do we have to
> > > care?
> > 
> > Oh, the first one that comes to mind is `cmd.exe`. You cannot start
> > `cmd.exe` with a UNC working directory without getting complaints.
> 
> Sure, but then again, why do we have to care?  Didn't you say GfW is
> using bash?

In particular, it affects all other native applications as well.  If that
home setting works for the user outside GfW/Cygwin, and given Cygwin apps
don't care, why should this suddenly be a problem for GfW?


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 --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Allow overriding the home directory via the HOME variable
  2015-10-23  9:41       ` Corinna Vinschen
@ 2015-10-23 12:00         ` Johannes Schindelin
  0 siblings, 0 replies; 25+ messages in thread
From: Johannes Schindelin @ 2015-10-23 12:00 UTC (permalink / raw)
  To: Corinna Vinschen; +Cc: cygwin-patches

Hi Corinna,

On Fri, 23 Oct 2015, Corinna Vinschen wrote:

> On Oct 23 11:10, Corinna Vinschen wrote:
> > On Oct 22 17:38, Johannes Schindelin wrote:
> > > 
> > > On Wed, 21 Oct 2015, Corinna Vinschen wrote:
> > > 
> > > > On Sep 16 15:06, Johannes Schindelin wrote:
> > > > > 	* uinfo.cc (cygheap_pwdgrp::get_home): Offer an option in
> > > > > 	nsswitch.conf that let's the environment variable HOME (or
> > > > > 	HOMEDRIVE & HOMEPATH, or USERPROFILE) define the home
> > > > > 	directory.
> > > > > 
> > > > > 	* ntsec.xml: Document the `env` schema.
> > > > > 
> > > > > Detailed comments:
> > > > > 
> > > > > In the context of Git for Windows, it is a well-established technique
> > > > > to let the `$HOME` variable define where the current user's home
> > > > > directory is, falling back to `$HOMEDRIVE$HOMEPATH` and `$USERPROFILE`.
> > > > > 
> > > > > The idea is that we want to share user-specific settings between
> > > > > programs, whether they be Cygwin, MSys2 or not.  Unfortunately, we
> > > > > cannot blindly activate the "db_home: windows" setting because in some
> > > > > setups, the user's home directory is set to a hidden directory via an
> > > > > UNC path (\\share\some\hidden\folder$) -- something many programs
> > > > > cannot handle correctly.
> > > > 
> > > > -v, please.  Which applications can't handle that?  Why do we have to
> > > > care?
> > > 
> > > Oh, the first one that comes to mind is `cmd.exe`. You cannot start
> > > `cmd.exe` with a UNC working directory without getting complaints.
> > 
> > Sure, but then again, why do we have to care?  Didn't you say GfW is
> > using bash?
> 
> In particular, it affects all other native applications as well.  If that
> home setting works for the user outside GfW/Cygwin, and given Cygwin apps
> don't care, why should this suddenly be a problem for GfW?

I did say that Git for Windows uses bash to execute the shell scripts that
are part of Git. However, this says nothing about the *entry* point into
Git for Windows, which is quite often `cmd.exe`. Even worse: users are
free to provide hooks as batch scripts, in which case you encounter that
bad bug again.

The point you make about getent is a good one, I did indeed misunderstand
under what circumstances the modified code path is hit. I will change it
so that it only triggers for the current user.

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2015-12-17 18:05 ` [PATCH v2 0/2] Support deriving the current user's home directory via HOME Johannes Schindelin
  2015-12-17 18:05   ` [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account Johannes Schindelin
@ 2015-12-17 18:05   ` Johannes Schindelin
  2015-12-17 20:20     ` Corinna Vinschen
  2022-09-21 11:51   ` [PATCH v3 0/3] Support deriving the current user's home directory via HOME Johannes Schindelin
  2 siblings, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2015-12-17 18:05 UTC (permalink / raw)
  To: cygwin-patches

	* uinfo.cc (cygheap_pwdgrp::get_home): Offer an option in
	nsswitch.conf that let's the environment variable HOME (or
	HOMEDRIVE & HOMEPATH, or USERPROFILE) define the home
	directory of the current user.

	* ntsec.xml: Document the `env` schema.

Detailed comments:

In the context of Git for Windows, it is a well-established technique
to let the `$HOME` variable define where the current user's home
directory is, falling back to `$HOMEDRIVE$HOMEPATH` and `$USERPROFILE`.

The idea is that we want to share user-specific settings between
programs, whether they be Cygwin, MSys2 or not.  Unfortunately, we
cannot blindly activate the "db_home: windows" setting because in some
setups, the user's home directory is set to a hidden directory via an
UNC path (\\share\some\hidden\folder$) -- something many programs
cannot handle correctly, e.g. cmd.exe and other native Windows
applications that users want to employ as Git helpers.

The established technique is to allow setting the user's home directory
via the environment variables mentioned above.  This has the additional
advantage that it is much faster than querying the Windows user database.

Of course this scheme needs to be opt-in.  For that reason, it needs
to be activated explicitly via `db_home: env` in `/etc/nsswitch.conf`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 winsup/cygwin/cygheap.h |  3 ++-
 winsup/cygwin/uinfo.cc  | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 winsup/doc/ntsec.xml    | 20 ++++++++++++++++++++
 3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h
index fd84814..097d50f 100644
--- a/winsup/cygwin/cygheap.h
+++ b/winsup/cygwin/cygheap.h
@@ -414,7 +414,8 @@ public:
     NSS_SCHEME_UNIX,
     NSS_SCHEME_DESC,
     NSS_SCHEME_PATH,
-    NSS_SCHEME_FREEATTR
+    NSS_SCHEME_FREEATTR,
+    NSS_SCHEME_ENV
   };
   struct nss_scheme_t {
     nss_scheme_method	method;
diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index c9b3e09..a5d6270 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -780,6 +780,8 @@ cygheap_pwdgrp::nss_init_line (const char *line)
 		    scheme[idx].method = NSS_SCHEME_UNIX;
 		  else if (NSS_CMP ("desc"))
 		    scheme[idx].method = NSS_SCHEME_DESC;
+		  else if (NSS_CMP ("env"))
+		    scheme[idx].method = NSS_SCHEME_ENV;
 		  else if (NSS_NCMP ("/"))
 		    {
 		      const char *e = c + strcspn (c, " \t");
@@ -970,6 +972,40 @@ fetch_from_path (cyg_ldap *pldap, PUSER_INFO_3 ui, cygpsid &sid, PCWSTR str,
   return ret;
 }
 
+static size_t
+fetch_env(LPCWSTR key, char *buf, size_t size)
+{
+  WCHAR wbuf[32767];
+  DWORD max = sizeof wbuf / sizeof *wbuf;
+  DWORD len = GetEnvironmentVariableW (key, wbuf, max);
+
+  if (!len || len >= max)
+    return 0;
+
+  len = sys_wcstombs (buf, size, wbuf, len);
+  return len && len < size ? len : 0;
+}
+
+static char *
+fetch_home_env (void)
+{
+  char home[32767];
+  size_t max = sizeof home / sizeof *home, len;
+
+  if (fetch_env (L"HOME", home, max)
+      || ((len = fetch_env (L"HOMEDRIVE", home, max))
+        && fetch_env (L"HOMEPATH", home + len, max - len))
+      || fetch_env (L"USERPROFILE", home, max))
+    {
+      tmp_pathbuf tp;
+      cygwin_conv_path (CCP_WIN_A_TO_POSIX | CCP_ABSOLUTE,
+	  home, tp.c_get(), NT_MAX_PATH);
+      return strdup(tp.c_get());
+    }
+
+  return NULL;
+}
+
 char *
 cygheap_pwdgrp::get_home (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 			  PCWSTR dnsdomain, PCWSTR name, bool full_qualified)
@@ -1029,6 +1065,10 @@ cygheap_pwdgrp::get_home (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 		}
 	    }
 	  break;
+	case NSS_SCHEME_ENV:
+	  if (RtlEqualSid (sid, cygheap->user.sid ()))
+	    home = fetch_home_env ();
+	  break;
 	}
     }
   return home;
@@ -1060,6 +1100,10 @@ cygheap_pwdgrp::get_home (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	  home = fetch_from_path (NULL, ui, sid, home_scheme[idx].attrib,
 				  dom, NULL, name, full_qualified);
 	  break;
+	case NSS_SCHEME_ENV:
+	  if (RtlEqualSid (sid, cygheap->user.sid ()))
+	    home = fetch_home_env ();
+	  break;
 	}
     }
   return home;
@@ -1079,6 +1123,7 @@ cygheap_pwdgrp::get_shell (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_FALLBACK:
 	  return NULL;
 	case NSS_SCHEME_WINDOWS:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_CYGWIN:
 	  if (pldap->fetch_ad_account (sid, false, dnsdomain))
@@ -1143,6 +1188,7 @@ cygheap_pwdgrp::get_shell (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_CYGWIN:
 	case NSS_SCHEME_UNIX:
 	case NSS_SCHEME_FREEATTR:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_DESC:
 	  shell = fetch_from_description (ui->usri3_comment, L"shell=\"", 7);
@@ -1223,6 +1269,8 @@ cygheap_pwdgrp::get_gecos (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 		sys_wcstombs_alloc (&gecos, HEAP_NOTHEAP, val);
 	    }
 	  break;
+	case NSS_SCHEME_ENV:
+	  break;
 	}
     }
   if (gecos)
@@ -1249,6 +1297,7 @@ cygheap_pwdgrp::get_gecos (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_CYGWIN:
 	case NSS_SCHEME_UNIX:
 	case NSS_SCHEME_FREEATTR:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_DESC:
 	  gecos = fetch_from_description (ui->usri3_comment, L"gecos=\"", 7);
diff --git a/winsup/doc/ntsec.xml b/winsup/doc/ntsec.xml
index ae0a119..14c37c5 100644
--- a/winsup/doc/ntsec.xml
+++ b/winsup/doc/ntsec.xml
@@ -1354,6 +1354,16 @@ schemata are the following:
 	      See <xref linkend="ntsec-mapping-nsswitch-desc"></xref>
 	      for a more detailed description.</listitem>
   </varlistentry>
+  <varlistentry>
+    <term><literal>env</literal></term>
+    <listitem>Derives the home directory of the current user from the
+	      environment variable <literal>HOME</literal> (falling back to
+	      <literal>HOMEDRIVE\HOMEPATH</literal> and
+	      <literal>USERPROFILE</literal>, in that order).  This is faster
+	      than the <term><literal>windows</literal></term> schema at the
+	      expense of determining only the current user's home directory
+	      correctly.</listitem>
+  </varlistentry>
 </variablelist>
 
 <para>
@@ -1487,6 +1497,16 @@ of each schema when used with <literal>db_home:</literal>
 	      for a detailed description.</listitem>
   </varlistentry>
   <varlistentry>
+    <term><literal>env</literal></term>
+    <listitem>Derives the home directory of the current user from the
+	      environment variable <literal>HOME</literal> (falling back to
+	      <literal>HOMEDRIVE\HOMEPATH</literal> and
+	      <literal>USERPROFILE</literal>, in that order).  This is faster
+	      than the <term><literal>windows</literal></term> schema at the
+	      expense of determining only the current user's home directory
+	      correctly.</listitem>
+  </varlistentry>
+  <varlistentry>
     <term><literal>@ad_attribute</literal></term>
     <listitem>AD only: The user's home directory is set to the path given
 	      in the <literal>ad_attribute</literal> attribute.  The path
-- 
2.6.3.windows.1.300.g1c25e49


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account
  2015-12-17 18:05 ` [PATCH v2 0/2] Support deriving the current user's home directory via HOME Johannes Schindelin
@ 2015-12-17 18:05   ` Johannes Schindelin
  2015-12-17 20:49     ` Corinna Vinschen
  2015-12-17 18:05   ` [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable Johannes Schindelin
  2022-09-21 11:51   ` [PATCH v3 0/3] Support deriving the current user's home directory via HOME Johannes Schindelin
  2 siblings, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2015-12-17 18:05 UTC (permalink / raw)
  To: cygwin-patches

We should not blindly set the home directory of the SYSTEM account to
/home/SYSTEM, especially not when that value disagrees with what is
configured via the `db_home` line in the `/etc/nsswitch.conf` file.

This fixes https://github.com/git-for-windows/git/issues/435

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 winsup/cygwin/uinfo.cc | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index a5d6270..8c51b82 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -2129,7 +2129,11 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
 	 it to a well-known group here. */
       if (acc_type == SidTypeUser
 	  && (sid_sub_auth_count (sid) <= 3 || sid_id_auth (sid) == 11))
-	acc_type = SidTypeWellKnownGroup;
+	{
+	  acc_type = SidTypeWellKnownGroup;
+	  home = cygheap->pg.get_home (pldap, sid, dom, domain, name,
+				       fully_qualified_name);
+	}
       switch (acc_type)
       	{
 	case SidTypeUser:
-- 
2.6.3.windows.1.300.g1c25e49

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2 0/2] Support deriving the current user's home directory via HOME
  2015-09-16 13:06 [PATCH] Allow overriding the home directory via the HOME variable Johannes Schindelin
  2015-10-21 18:32 ` Corinna Vinschen
@ 2015-12-17 18:05 ` Johannes Schindelin
  2015-12-17 18:05   ` [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account Johannes Schindelin
                     ` (2 more replies)
  1 sibling, 3 replies; 25+ messages in thread
From: Johannes Schindelin @ 2015-12-17 18:05 UTC (permalink / raw)
  To: cygwin-patches

This patch mini-series supports Git for Windows' main strategy to
determine the current user's home directory by looking at the
environment variable HOME, falling back to HOMEDRIVE and HOMEPATH, and
if these variables are also unset, to USERPROFILE.

This strategy is a quick method to determine the home directory, and it
also allows users to override the home directory easily (e.g. in case
that their real home directory is a network share that is not all that
well handled by some commands such as cmd.exe's cd command).

Sorry for sending out v2 so late...!


Johannes Schindelin (2):
  Allow deriving the current user's home directory via the HOME variable
  Respect `db_home` setting even for the SYSTEM account

 winsup/cygwin/cygheap.h |  3 ++-
 winsup/cygwin/uinfo.cc  | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-
 winsup/doc/ntsec.xml    | 20 ++++++++++++++++++
 3 files changed, 76 insertions(+), 2 deletions(-)

Interdiff vs v1:

diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index 525a90e..8c51b82 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -1066,7 +1066,8 @@ cygheap_pwdgrp::get_home (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 	    }
 	  break;
 	case NSS_SCHEME_ENV:
-	  home = fetch_home_env ();
+	  if (RtlEqualSid (sid, cygheap->user.sid ()))
+	    home = fetch_home_env ();
 	  break;
 	}
     }
@@ -1100,7 +1101,8 @@ cygheap_pwdgrp::get_home (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 				  dom, NULL, name, full_qualified);
 	  break;
 	case NSS_SCHEME_ENV:
-	  home = fetch_home_env ();
+	  if (RtlEqualSid (sid, cygheap->user.sid ()))
+	    home = fetch_home_env ();
 	  break;
 	}
     }
@@ -2127,7 +2129,11 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
 	 it to a well-known group here. */
       if (acc_type == SidTypeUser
 	  && (sid_sub_auth_count (sid) <= 3 || sid_id_auth (sid) == 11))
-	acc_type = SidTypeWellKnownGroup;
+	{
+	  acc_type = SidTypeWellKnownGroup;
+	  home = cygheap->pg.get_home (pldap, sid, dom, domain, name,
+				       fully_qualified_name);
+	}
       switch (acc_type)
       	{
 	case SidTypeUser:
diff --git a/winsup/doc/ntsec.xml b/winsup/doc/ntsec.xml
index 9cee58c..14c37c5 100644
--- a/winsup/doc/ntsec.xml
+++ b/winsup/doc/ntsec.xml
@@ -1356,8 +1356,8 @@ schemata are the following:
   </varlistentry>
   <varlistentry>
     <term><literal>env</literal></term>
-    <listitem>Derives the home directory from the environment variable
-	      <literal>HOME</literal> (falling back to
+    <listitem>Derives the home directory of the current user from the
+	      environment variable <literal>HOME</literal> (falling back to
 	      <literal>HOMEDRIVE\HOMEPATH</literal> and
 	      <literal>USERPROFILE</literal>, in that order).  This is faster
 	      than the <term><literal>windows</literal></term> schema at the
@@ -1498,8 +1498,8 @@ of each schema when used with <literal>db_home:</literal>
   </varlistentry>
   <varlistentry>
     <term><literal>env</literal></term>
-    <listitem>Derives the home directory from the environment variable
-	      <literal>HOME</literal> (falling back to
+    <listitem>Derives the home directory of the current user from the
+	      environment variable <literal>HOME</literal> (falling back to
 	      <literal>HOMEDRIVE\HOMEPATH</literal> and
 	      <literal>USERPROFILE</literal>, in that order).  This is faster
 	      than the <term><literal>windows</literal></term> schema at the

-- 
2.6.3.windows.1.300.g1c25e49

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2015-12-17 18:05   ` [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable Johannes Schindelin
@ 2015-12-17 20:20     ` Corinna Vinschen
  2022-09-21 11:58       ` Johannes Schindelin
  0 siblings, 1 reply; 25+ messages in thread
From: Corinna Vinschen @ 2015-12-17 20:20 UTC (permalink / raw)
  To: cygwin-patches

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

Hi Johannes,

a few comments...

On Dec 17 19:05, Johannes Schindelin wrote:
> [...]
> diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> index c9b3e09..a5d6270 100644
> --- a/winsup/cygwin/uinfo.cc
> +++ b/winsup/cygwin/uinfo.cc
> [...]
> +static size_t
> +fetch_env(LPCWSTR key, char *buf, size_t size)
           ^^^
           space

> +{
> +  WCHAR wbuf[32767];

Ok, there are a couple of problems here.  First, since this buffer
is a filename buffer, use NT_MAX_PATH from winsup.h as buffer size.

But then again, please avoid allocating 64K buffers on the stack.
That's what tmp_pathbuf:w_get () is for.

> +  DWORD max = sizeof wbuf / sizeof *wbuf;
> +  DWORD len = GetEnvironmentVariableW (key, wbuf, max);

This call to GetEnvironmentVariableW looks gratuitous to me.  Why don't
you simply call getenv?  It did the entire job already, it avoids the
requirement for a local buffer, and in case of $HOME it even did the
Win32->POSIX path conversion.  If there's a really good reason for using
GetEnvironmentVariableW it begs at least for a longish comment.

> +
> +  if (!len || len >= max)
> +    return 0;
> +
> +  len = sys_wcstombs (buf, size, wbuf, len);
> +  return len && len < size ? len : 0;
> +}
> +
> +static char *
> +fetch_home_env (void)
> +{
> +  char home[32767];
> +  size_t max = sizeof home / sizeof *home, len;
> +
> +  if (fetch_env (L"HOME", home, max)
> +      || ((len = fetch_env (L"HOMEDRIVE", home, max))
> +        && fetch_env (L"HOMEPATH", home + len, max - len))
> +      || fetch_env (L"USERPROFILE", home, max))
> +    {
> +      tmp_pathbuf tp;
> +      cygwin_conv_path (CCP_WIN_A_TO_POSIX | CCP_ABSOLUTE,
> +	  home, tp.c_get(), NT_MAX_PATH);
                       ^^^
                       space
> +      return strdup(tp.c_get());
                     ^^^      ^^^
                     space......s

Whoa, tp.c_get() twice to access the same space?  That's a dirty trick
which may puzzle later readers of the code and heavily depends on
knowing the internals of tmp_pathbuf.  Please use a variable and only
assign tp.c_get () once.

OTOH, the above's a case for a cygwin_create_path call, rather than
cygwin_conv_path+strdup.  Also, if there's *really* a good reason to use
GetEnvironmentVariableW, you should collapse sys_wcstombs+cygwin_conv_path+
strdup into a single cygwin_create_path (CCP_WIN_W_TO_POSIX, ...).

> [...]
> @@ -1079,6 +1123,7 @@ cygheap_pwdgrp::get_shell (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
>  	case NSS_SCHEME_FALLBACK:
>  	  return NULL;
>  	case NSS_SCHEME_WINDOWS:
> +	case NSS_SCHEME_ENV:
>  	  break;
>  	case NSS_SCHEME_CYGWIN:
>  	  if (pldap->fetch_ad_account (sid, false, dnsdomain))

You know that I don't exactly like the "env" idea, but if we implement
it anyway, wouldn't it make sense to add some kind of $SHELL handling as
well, for symmetry?

> [...]
> @@ -1487,6 +1497,16 @@ of each schema when used with <literal>db_home:</literal>
>  	      for a detailed description.</listitem>
>    </varlistentry>
>    <varlistentry>
> +    <term><literal>env</literal></term>
> +    <listitem>Derives the home directory of the current user from the
> +	      environment variable <literal>HOME</literal> (falling back to
> +	      <literal>HOMEDRIVE\HOMEPATH</literal> and
> +	      <literal>USERPROFILE</literal>, in that order).  This is faster
> +	      than the <term><literal>windows</literal></term> schema at the
> +	      expense of determining only the current user's home directory
> +	      correctly.</listitem>

In both case of the documentation it might make sense to add a few words
along the lines of "This schema is skipped for any other account",
wouldn't it?


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 --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account
  2015-12-17 18:05   ` [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account Johannes Schindelin
@ 2015-12-17 20:49     ` Corinna Vinschen
  2015-12-17 21:02       ` Corinna Vinschen
  2022-09-21 12:00       ` Johannes Schindelin
  0 siblings, 2 replies; 25+ messages in thread
From: Corinna Vinschen @ 2015-12-17 20:49 UTC (permalink / raw)
  To: cygwin-patches

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

On Dec 17 19:05, Johannes Schindelin wrote:
> We should not blindly set the home directory of the SYSTEM account to
> /home/SYSTEM, especially not when that value disagrees with what is
> configured via the `db_home` line in the `/etc/nsswitch.conf` file.
> 
> This fixes https://github.com/git-for-windows/git/issues/435
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  winsup/cygwin/uinfo.cc | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> index a5d6270..8c51b82 100644
> --- a/winsup/cygwin/uinfo.cc
> +++ b/winsup/cygwin/uinfo.cc
> @@ -2129,7 +2129,11 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
>  	 it to a well-known group here. */
>        if (acc_type == SidTypeUser
>  	  && (sid_sub_auth_count (sid) <= 3 || sid_id_auth (sid) == 11))
> -	acc_type = SidTypeWellKnownGroup;
> +	{
> +	  acc_type = SidTypeWellKnownGroup;
> +	  home = cygheap->pg.get_home (pldap, sid, dom, domain, name,
> +				       fully_qualified_name);

Uhm, that's a bit over the top, isn't it?  It will affect all S-1-5-X
accounts as well as the S-1-5-11 Windows account SIDs.  Is that really
what you want?

Using pldap here may SEGV in cygheap_pwdgrp::get_home, btw, because
it may be NULL.  cygheap_pwdgrp::get_home doesn't check pldap for
validity, it expects a valid pointer.  You could either use cldap, or
cygheap_pwdgrp::get_home would have to check pldap before using it.

However, either way there's another problem: Independently of the
configured db_home schemes, you don't want to ask the DC for info on
these builtin accounts.  The better approach might be to call the
PUSER_INFO_3 variant of cygheap_pwdgrp::get_home with a NULL ui
pointer and just check for ui in the NSS_SCHEME_DESC case.  The other
called functions fetch_windows_home and fetch_from_path both can
live with both pointers, pldap and ui being NULL.


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 --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account
  2015-12-17 20:49     ` Corinna Vinschen
@ 2015-12-17 21:02       ` Corinna Vinschen
  2022-09-21 12:00       ` Johannes Schindelin
  1 sibling, 0 replies; 25+ messages in thread
From: Corinna Vinschen @ 2015-12-17 21:02 UTC (permalink / raw)
  To: cygwin-patches

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

On Dec 17 21:49, Corinna Vinschen wrote:
> On Dec 17 19:05, Johannes Schindelin wrote:
> > We should not blindly set the home directory of the SYSTEM account to
> > /home/SYSTEM, especially not when that value disagrees with what is
> > configured via the `db_home` line in the `/etc/nsswitch.conf` file.
> > 
> > This fixes https://github.com/git-for-windows/git/issues/435
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  winsup/cygwin/uinfo.cc | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> > index a5d6270..8c51b82 100644
> > --- a/winsup/cygwin/uinfo.cc
> > +++ b/winsup/cygwin/uinfo.cc
> > @@ -2129,7 +2129,11 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
> >  	 it to a well-known group here. */
> >        if (acc_type == SidTypeUser
> >  	  && (sid_sub_auth_count (sid) <= 3 || sid_id_auth (sid) == 11))
> > -	acc_type = SidTypeWellKnownGroup;
> > +	{
> > +	  acc_type = SidTypeWellKnownGroup;
> > +	  home = cygheap->pg.get_home (pldap, sid, dom, domain, name,
> > +				       fully_qualified_name);
> 
> Uhm, that's a bit over the top, isn't it?  It will affect all S-1-5-X
> accounts as well as the S-1-5-11 Windows account SIDs.  Is that really

s/S-1-5-11/S-1-11/, sorry.

> what you want?
> 
> Using pldap here may SEGV in cygheap_pwdgrp::get_home, btw, because
> it may be NULL.  cygheap_pwdgrp::get_home doesn't check pldap for
> validity, it expects a valid pointer.  You could either use cldap, or
> cygheap_pwdgrp::get_home would have to check pldap before using it.
> 
> However, either way there's another problem: Independently of the
> configured db_home schemes, you don't want to ask the DC for info on
> these builtin accounts.  The better approach might be to call the
> PUSER_INFO_3 variant of cygheap_pwdgrp::get_home with a NULL ui
> pointer and just check for ui in the NSS_SCHEME_DESC case.  The other
> called functions fetch_windows_home and fetch_from_path both can
> live with both pointers, pldap and ui being NULL.

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 --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 0/3] Support deriving the current user's home directory via HOME
  2015-12-17 18:05 ` [PATCH v2 0/2] Support deriving the current user's home directory via HOME Johannes Schindelin
  2015-12-17 18:05   ` [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account Johannes Schindelin
  2015-12-17 18:05   ` [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable Johannes Schindelin
@ 2022-09-21 11:51   ` Johannes Schindelin
  2022-09-21 11:51     ` [PATCH v3 1/3] Allow deriving the current user's home directory via the HOME variable Johannes Schindelin
                       ` (2 more replies)
  2 siblings, 3 replies; 25+ messages in thread
From: Johannes Schindelin @ 2022-09-21 11:51 UTC (permalink / raw)
  To: cygwin-patches

This patch mini-series supports Git for Windows' main strategy to
determine the current user's home directory by looking at the
environment variable HOME, falling back to HOMEDRIVE and HOMEPATH, and
if these variables are also unset, to USERPROFILE.

This strategy is a quick method to determine the home directory, and it
also allows users to override the home directory easily (e.g. in case
that their real home directory is a network share that is not all that
well handled by some commands such as cmd.exe's cd command).

Sorry for sending out v3 sooooo late...!

Changes since v2:
- Using `getenv()` and `cygwin_create_path()` instead of the
  `GetEnvironmentVariableW()`/`cygwin_conv_path()` dance
- Adjusted the documentation to drive home that this only affects the
  _current_ user's home directory
- Using the `PUSER_INFO_3` variant of `get_home()`
- Adjusted the commit messages
- Added another patch, to support "ad-hoc cloud accounts"

Johannes Schindelin (3):
  Allow deriving the current user's home directory via the HOME variable
  Respect `db_home` setting even for SYSTEM/Microsoft accounts
  Respect `db_home: env` even when no uid can be determined

 winsup/cygwin/local_includes/cygheap.h |  3 +-
 winsup/cygwin/uinfo.cc                 | 54 ++++++++++++++++++++++++--
 winsup/doc/ntsec.xml                   | 22 +++++++++++
 3 files changed, 75 insertions(+), 4 deletions(-)

Range-diff:
1:  047fe1d78c ! 1:  6f8fe89d9d Allow deriving the current user's home directory via the HOME variable
    @@ Metadata
      ## Commit message ##
         Allow deriving the current user's home directory via the HOME variable

    -            * uinfo.cc (cygheap_pwdgrp::get_home): Offer an option in
    -            nsswitch.conf that let's the environment variable HOME (or
    -            HOMEDRIVE & HOMEPATH, or USERPROFILE) define the home
    -            directory of the current user.
    -
    -            * ntsec.xml: Document the `env` schema.
    -
    -    Detailed comments:
    -
    -    In the context of Git for Windows, it is a well-established technique
    -    to let the `$HOME` variable define where the current user's home
    -    directory is, falling back to `$HOMEDRIVE$HOMEPATH` and `$USERPROFILE`.
    +    This patch hails from Git for Windows (where the Cygwin runtime is used
    +    in the form of a slightly modified MSYS2 runtime), where it is a
    +    well-established technique to let the `$HOME` variable define where the
    +    current user's home directory is, falling back to `$HOMEDRIVE$HOMEPATH`
    +    and `$USERPROFILE`.

         The idea is that we want to share user-specific settings between
    -    programs, whether they be Cygwin, MSys2 or not.  Unfortunately, we
    +    programs, whether they be Cygwin, MSYS2 or not.  Unfortunately, we
         cannot blindly activate the "db_home: windows" setting because in some
         setups, the user's home directory is set to a hidden directory via an
         UNC path (\\share\some\hidden\folder$) -- something many programs
    -    cannot handle correctly, e.g. cmd.exe and other native Windows
    +    cannot handle correctly, e.g. `cmd.exe` and other native Windows
         applications that users want to employ as Git helpers.

         The established technique is to allow setting the user's home directory
    -    via the environment variables mentioned above.  This has the additional
    -    advantage that it is much faster than querying the Windows user database.
    +    via the environment variables mentioned above: `$HOMEDRIVE$HOMEPATH` or
    +    `$USERPROFILE`.  This has the additional advantage that it is much
    +    faster than querying the Windows user database.

         Of course this scheme needs to be opt-in.  For that reason, it needs
         to be activated explicitly via `db_home: env` in `/etc/nsswitch.conf`.

         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

    - ## winsup/cygwin/cygheap.h ##
    -@@ winsup/cygwin/cygheap.h: public:
    + ## winsup/cygwin/local_includes/cygheap.h ##
    +@@ winsup/cygwin/local_includes/cygheap.h: public:
          NSS_SCHEME_UNIX,
          NSS_SCHEME_DESC,
          NSS_SCHEME_PATH,
    @@ winsup/cygwin/uinfo.cc: fetch_from_path (cyg_ldap *pldap, PUSER_INFO_3 ui, cygps
        return ret;
      }

    -+static size_t
    -+fetch_env(LPCWSTR key, char *buf, size_t size)
    -+{
    -+  WCHAR wbuf[32767];
    -+  DWORD max = sizeof wbuf / sizeof *wbuf;
    -+  DWORD len = GetEnvironmentVariableW (key, wbuf, max);
    -+
    -+  if (!len || len >= max)
    -+    return 0;
    -+
    -+  len = sys_wcstombs (buf, size, wbuf, len);
    -+  return len && len < size ? len : 0;
    -+}
    -+
     +static char *
     +fetch_home_env (void)
     +{
    -+  char home[32767];
    -+  size_t max = sizeof home / sizeof *home, len;
    ++  tmp_pathbuf tp;
    ++  char *p, *q;
    ++  const char *home, *home_drive, *home_path;
     +
    -+  if (fetch_env (L"HOME", home, max)
    -+      || ((len = fetch_env (L"HOMEDRIVE", home, max))
    -+        && fetch_env (L"HOMEPATH", home + len, max - len))
    -+      || fetch_env (L"USERPROFILE", home, max))
    -+    {
    -+      tmp_pathbuf tp;
    -+      cygwin_conv_path (CCP_WIN_A_TO_POSIX | CCP_ABSOLUTE,
    -+	  home, tp.c_get(), NT_MAX_PATH);
    -+      return strdup(tp.c_get());
    -+    }
    ++  if ((home = getenv ("HOME"))
    ++      || ((home_drive = getenv ("HOMEDRIVE"))
    ++	  && (home_path = getenv ("HOMEPATH"))
    ++	  // concatenate HOMEDRIVE and HOMEPATH
    ++          && (home = p = tp.c_get ())
    ++	  && (q = stpncpy (p, home_drive, NT_MAX_PATH))
    ++          && strlcpy (q, home_path, NT_MAX_PATH - (q - p)))
    ++      || (home = getenv ("USERPROFILE")))
    ++    return (char *) cygwin_create_path (CCP_WIN_A_TO_POSIX, home);
     +
     +  return NULL;
     +}
    @@ winsup/cygwin/uinfo.cc: cygheap_pwdgrp::get_shell (PUSER_INFO_3 ui, cygpsid &sid
     +	case NSS_SCHEME_ENV:
      	  break;
      	case NSS_SCHEME_DESC:
    - 	  shell = fetch_from_description (ui->usri3_comment, L"shell=\"", 7);
    + 	  if (ui)
     @@ winsup/cygwin/uinfo.cc: cygheap_pwdgrp::get_gecos (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
      		sys_wcstombs_alloc (&gecos, HEAP_NOTHEAP, val);
      	    }
    @@ winsup/cygwin/uinfo.cc: cygheap_pwdgrp::get_gecos (PUSER_INFO_3 ui, cygpsid &sid
     +	case NSS_SCHEME_ENV:
      	  break;
      	case NSS_SCHEME_DESC:
    - 	  gecos = fetch_from_description (ui->usri3_comment, L"gecos=\"", 7);
    + 	  if (ui)

      ## winsup/doc/ntsec.xml ##
     @@ winsup/doc/ntsec.xml: schemata are the following:
    @@ winsup/doc/ntsec.xml: schemata are the following:
     +	      <literal>USERPROFILE</literal>, in that order).  This is faster
     +	      than the <term><literal>windows</literal></term> schema at the
     +	      expense of determining only the current user's home directory
    -+	      correctly.</listitem>
    ++	      correctly.  This schema is skipped for any other account.
    ++	      </listitem>
     +  </varlistentry>
      </variablelist>

    @@ winsup/doc/ntsec.xml: of each schema when used with <literal>db_home:</literal>
     +	      <literal>USERPROFILE</literal>, in that order).  This is faster
     +	      than the <term><literal>windows</literal></term> schema at the
     +	      expense of determining only the current user's home directory
    -+	      correctly.</listitem>
    ++	      correctly.  This schema is skipped for any other account.
    ++	      </listitem>
     +  </varlistentry>
        <varlistentry>
          <term><literal>@ad_attribute</literal></term>
2:  90c5b45fbe ! 2:  1b4ee89aa7 Respect `db_home` setting even for the SYSTEM account
    @@ Metadata
     Author: Johannes Schindelin <johannes.schindelin@gmx.de>

      ## Commit message ##
    -    Respect `db_home` setting even for the SYSTEM account
    +    Respect `db_home` setting even for SYSTEM/Microsoft accounts

    -    We should not blindly set the home directory of the SYSTEM account to
    -    /home/SYSTEM, especially not when that value disagrees with what is
    -    configured via the `db_home` line in the `/etc/nsswitch.conf` file.
    -
    -    This fixes https://github.com/git-for-windows/git/issues/435
    +    We should not blindly set the home directory of the SYSTEM account (or
    +    of Microsoft accounts) to /home/SYSTEM, especially not when that value
    +    disagrees with what is configured via the `db_home` line in the
    +    `/etc/nsswitch.conf` file.

         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

    @@ winsup/cygwin/uinfo.cc: pwdgrp::fetch_account_from_windows (fetch_user_arg_t &ar
     -	acc_type = SidTypeWellKnownGroup;
     +	{
     +	  acc_type = SidTypeWellKnownGroup;
    -+	  home = cygheap->pg.get_home (pldap, sid, dom, domain, name,
    ++	  home = cygheap->pg.get_home ((PUSER_INFO_3) NULL, sid, dom, name,
     +				       fully_qualified_name);
     +	}
    -       switch (acc_type)
    -       	{
    +       switch ((int) acc_type)
    + 	{
      	case SidTypeUser:
-:  ---------- > 3:  4d90319e44 Respect `db_home: env` even when no uid can be determined

base-commit: a68e99f8839e4697790077c8a77b506d528cc674
Published-As: https://github.com/dscho/msys2-runtime/releases/tag/home-env-cygwin-v3
Fetch-It-Via: git fetch https://github.com/dscho/msys2-runtime home-env-cygwin-v3

--
2.38.0.rc0.windows.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 1/3] Allow deriving the current user's home directory via the HOME variable
  2022-09-21 11:51   ` [PATCH v3 0/3] Support deriving the current user's home directory via HOME Johannes Schindelin
@ 2022-09-21 11:51     ` Johannes Schindelin
  2022-09-21 11:52     ` [PATCH v3 2/3] Respect `db_home` setting even for SYSTEM/Microsoft accounts Johannes Schindelin
  2022-09-21 11:52     ` [PATCH v3 3/3] Respect `db_home: env` even when no uid can be determined Johannes Schindelin
  2 siblings, 0 replies; 25+ messages in thread
From: Johannes Schindelin @ 2022-09-21 11:51 UTC (permalink / raw)
  To: cygwin-patches

This patch hails from Git for Windows (where the Cygwin runtime is used
in the form of a slightly modified MSYS2 runtime), where it is a
well-established technique to let the `$HOME` variable define where the
current user's home directory is, falling back to `$HOMEDRIVE$HOMEPATH`
and `$USERPROFILE`.

The idea is that we want to share user-specific settings between
programs, whether they be Cygwin, MSYS2 or not.  Unfortunately, we
cannot blindly activate the "db_home: windows" setting because in some
setups, the user's home directory is set to a hidden directory via an
UNC path (\\share\some\hidden\folder$) -- something many programs
cannot handle correctly, e.g. `cmd.exe` and other native Windows
applications that users want to employ as Git helpers.

The established technique is to allow setting the user's home directory
via the environment variables mentioned above: `$HOMEDRIVE$HOMEPATH` or
`$USERPROFILE`.  This has the additional advantage that it is much
faster than querying the Windows user database.

Of course this scheme needs to be opt-in.  For that reason, it needs
to be activated explicitly via `db_home: env` in `/etc/nsswitch.conf`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 winsup/cygwin/local_includes/cygheap.h |  3 ++-
 winsup/cygwin/uinfo.cc                 | 35 ++++++++++++++++++++++++++
 winsup/doc/ntsec.xml                   | 22 ++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/local_includes/cygheap.h b/winsup/cygwin/local_includes/cygheap.h
index 6a844babdb..444ca8b55c 100644
--- a/winsup/cygwin/local_includes/cygheap.h
+++ b/winsup/cygwin/local_includes/cygheap.h
@@ -408,7 +408,8 @@ public:
     NSS_SCHEME_UNIX,
     NSS_SCHEME_DESC,
     NSS_SCHEME_PATH,
-    NSS_SCHEME_FREEATTR
+    NSS_SCHEME_FREEATTR,
+    NSS_SCHEME_ENV
   };
   struct nss_scheme_t {
     nss_scheme_method	method;
diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index ce997c0f82..6e673ee39e 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -754,6 +754,8 @@ cygheap_pwdgrp::nss_init_line (const char *line)
 		    scheme[idx].method = NSS_SCHEME_UNIX;
 		  else if (NSS_CMP ("desc"))
 		    scheme[idx].method = NSS_SCHEME_DESC;
+		  else if (NSS_CMP ("env"))
+		    scheme[idx].method = NSS_SCHEME_ENV;
 		  else if (NSS_NCMP ("/"))
 		    {
 		      const char *e = c + strcspn (c, " \t");
@@ -942,6 +944,26 @@ fetch_from_path (cyg_ldap *pldap, PUSER_INFO_3 ui, cygpsid &sid, PCWSTR str,
   return ret;
 }

+static char *
+fetch_home_env (void)
+{
+  tmp_pathbuf tp;
+  char *p, *q;
+  const char *home, *home_drive, *home_path;
+
+  if ((home = getenv ("HOME"))
+      || ((home_drive = getenv ("HOMEDRIVE"))
+	  && (home_path = getenv ("HOMEPATH"))
+	  // concatenate HOMEDRIVE and HOMEPATH
+          && (home = p = tp.c_get ())
+	  && (q = stpncpy (p, home_drive, NT_MAX_PATH))
+          && strlcpy (q, home_path, NT_MAX_PATH - (q - p)))
+      || (home = getenv ("USERPROFILE")))
+    return (char *) cygwin_create_path (CCP_WIN_A_TO_POSIX, home);
+
+  return NULL;
+}
+
 char *
 cygheap_pwdgrp::get_home (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 			  PCWSTR dnsdomain, PCWSTR name, bool full_qualified)
@@ -1001,6 +1023,10 @@ cygheap_pwdgrp::get_home (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 		}
 	    }
 	  break;
+	case NSS_SCHEME_ENV:
+	  if (RtlEqualSid (sid, cygheap->user.sid ()))
+	    home = fetch_home_env ();
+	  break;
 	}
     }
   return home;
@@ -1033,6 +1059,10 @@ cygheap_pwdgrp::get_home (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	  home = fetch_from_path (NULL, ui, sid, home_scheme[idx].attrib,
 				  dom, NULL, name, full_qualified);
 	  break;
+	case NSS_SCHEME_ENV:
+	  if (RtlEqualSid (sid, cygheap->user.sid ()))
+	    home = fetch_home_env ();
+	  break;
 	}
     }
   return home;
@@ -1052,6 +1082,7 @@ cygheap_pwdgrp::get_shell (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_FALLBACK:
 	  return NULL;
 	case NSS_SCHEME_WINDOWS:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_CYGWIN:
 	  if (pldap->fetch_ad_account (sid, false, dnsdomain))
@@ -1116,6 +1147,7 @@ cygheap_pwdgrp::get_shell (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_CYGWIN:
 	case NSS_SCHEME_UNIX:
 	case NSS_SCHEME_FREEATTR:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_DESC:
 	  if (ui)
@@ -1197,6 +1229,8 @@ cygheap_pwdgrp::get_gecos (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
 		sys_wcstombs_alloc (&gecos, HEAP_NOTHEAP, val);
 	    }
 	  break;
+	case NSS_SCHEME_ENV:
+	  break;
 	}
     }
   if (gecos)
@@ -1223,6 +1257,7 @@ cygheap_pwdgrp::get_gecos (PUSER_INFO_3 ui, cygpsid &sid, PCWSTR dom,
 	case NSS_SCHEME_CYGWIN:
 	case NSS_SCHEME_UNIX:
 	case NSS_SCHEME_FREEATTR:
+	case NSS_SCHEME_ENV:
 	  break;
 	case NSS_SCHEME_DESC:
 	  if (ui)
diff --git a/winsup/doc/ntsec.xml b/winsup/doc/ntsec.xml
index d089964660..c02b1f72e1 100644
--- a/winsup/doc/ntsec.xml
+++ b/winsup/doc/ntsec.xml
@@ -1359,6 +1359,17 @@ schemata are the following:
 	      See <xref linkend="ntsec-mapping-nsswitch-desc"></xref>
 	      for a more detailed description.</listitem>
   </varlistentry>
+  <varlistentry>
+    <term><literal>env</literal></term>
+    <listitem>Derives the home directory of the current user from the
+	      environment variable <literal>HOME</literal> (falling back to
+	      <literal>HOMEDRIVE\HOMEPATH</literal> and
+	      <literal>USERPROFILE</literal>, in that order).  This is faster
+	      than the <term><literal>windows</literal></term> schema at the
+	      expense of determining only the current user's home directory
+	      correctly.  This schema is skipped for any other account.
+	      </listitem>
+  </varlistentry>
 </variablelist>

 <para>
@@ -1491,6 +1502,17 @@ of each schema when used with <literal>db_home:</literal>
 	      See <xref linkend="ntsec-mapping-nsswitch-desc"></xref>
 	      for a detailed description.</listitem>
   </varlistentry>
+  <varlistentry>
+    <term><literal>env</literal></term>
+    <listitem>Derives the home directory of the current user from the
+	      environment variable <literal>HOME</literal> (falling back to
+	      <literal>HOMEDRIVE\HOMEPATH</literal> and
+	      <literal>USERPROFILE</literal>, in that order).  This is faster
+	      than the <term><literal>windows</literal></term> schema at the
+	      expense of determining only the current user's home directory
+	      correctly.  This schema is skipped for any other account.
+	      </listitem>
+  </varlistentry>
   <varlistentry>
     <term><literal>@ad_attribute</literal></term>
     <listitem>AD only: The user's home directory is set to the path given
--
2.38.0.rc0.windows.1



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 2/3] Respect `db_home` setting even for SYSTEM/Microsoft accounts
  2022-09-21 11:51   ` [PATCH v3 0/3] Support deriving the current user's home directory via HOME Johannes Schindelin
  2022-09-21 11:51     ` [PATCH v3 1/3] Allow deriving the current user's home directory via the HOME variable Johannes Schindelin
@ 2022-09-21 11:52     ` Johannes Schindelin
  2022-09-21 11:52     ` [PATCH v3 3/3] Respect `db_home: env` even when no uid can be determined Johannes Schindelin
  2 siblings, 0 replies; 25+ messages in thread
From: Johannes Schindelin @ 2022-09-21 11:52 UTC (permalink / raw)
  To: cygwin-patches

We should not blindly set the home directory of the SYSTEM account (or
of Microsoft accounts) to /home/SYSTEM, especially not when that value
disagrees with what is configured via the `db_home` line in the
`/etc/nsswitch.conf` file.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 winsup/cygwin/uinfo.cc | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index 6e673ee39e..5e4243fa4e 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -2253,7 +2253,11 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
 	 it to a well-known group here. */
       if (acc_type == SidTypeUser
 	  && (sid_sub_auth_count (sid) <= 3 || sid_id_auth (sid) == 11))
-	acc_type = SidTypeWellKnownGroup;
+	{
+	  acc_type = SidTypeWellKnownGroup;
+	  home = cygheap->pg.get_home ((PUSER_INFO_3) NULL, sid, dom, name,
+				       fully_qualified_name);
+	}
       switch ((int) acc_type)
 	{
 	case SidTypeUser:
--
2.38.0.rc0.windows.1



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 3/3] Respect `db_home: env` even when no uid can be determined
  2022-09-21 11:51   ` [PATCH v3 0/3] Support deriving the current user's home directory via HOME Johannes Schindelin
  2022-09-21 11:51     ` [PATCH v3 1/3] Allow deriving the current user's home directory via the HOME variable Johannes Schindelin
  2022-09-21 11:52     ` [PATCH v3 2/3] Respect `db_home` setting even for SYSTEM/Microsoft accounts Johannes Schindelin
@ 2022-09-21 11:52     ` Johannes Schindelin
  2 siblings, 0 replies; 25+ messages in thread
From: Johannes Schindelin @ 2022-09-21 11:52 UTC (permalink / raw)
  To: cygwin-patches

In particular when we cannot figure out a uid for the current user, we
should still respect the `db_home: env` setting. Such a situation occurs
for example when the domain returned by `LookupAccountSid()` is not our
machine name and at the same time our machine is no domain member: In
that case, we have nobody to ask for the POSIX offset necessary to come
up with the uid.

It is important that even in such cases, the `HOME` environment variable
can be used to override the home directory, e.g. when Git for Windows is
used by an account that was generated on the fly, e.g. for transient use
in a cloud scenario.

Reported by David Ebbo.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 winsup/cygwin/uinfo.cc | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index 5e4243fa4e..b79c2b265d 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -904,6 +904,8 @@ fetch_from_path (cyg_ldap *pldap, PUSER_INFO_3 ui, cygpsid &sid, PCWSTR str,
 	    case L'u':
 	      if (full_qualified)
 		{
+		  if (!dom)
+		    break;
 		  w = wcpncpy (w, dom, we - w);
 		  if (w < we)
 		    *w++ = cygheap->pg.nss_separator ()[0];
@@ -914,6 +916,8 @@ fetch_from_path (cyg_ldap *pldap, PUSER_INFO_3 ui, cygpsid &sid, PCWSTR str,
 	      w = wcpncpy (w, name, we - w);
 	      break;
 	    case L'D':
+	      if (!dom)
+		break;
 	      w = wcpncpy (w, dom, we - w);
 	      break;
 	    case L'H':
@@ -2200,6 +2204,10 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
 	{
 	  /* Just some fake. */
 	  sid = csid.create (99, 1, 0);
+	  if (arg.id == cygheap->user.real_uid)
+	    home = cygheap->pg.get_home ((PUSER_INFO_3) NULL,
+					 cygheap->user.sid(),
+					 NULL, NULL, false);
 	  break;
 	}
       else if (arg.id >= UNIX_POSIX_OFFSET)
@@ -2760,10 +2768,11 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
      logon.  Unless it's the SYSTEM account.  This conveniently allows to
      logon interactively as SYSTEM for debugging purposes. */
   else if (acc_type != SidTypeUser && sid != well_known_system_sid)
-    __small_sprintf (linebuf, "%W:*:%u:%u:U-%W\\%W,%s:/:/sbin/nologin",
+    __small_sprintf (linebuf, "%W:*:%u:%u:U-%W\\%W,%s:%s:/sbin/nologin",
 		     posix_name, uid, gid,
 		     dom, name,
-		     sid.string ((char *) sidstr));
+		     sid.string ((char *) sidstr),
+		     home ? home : "/");
   else
     __small_sprintf (linebuf, "%W:*:%u:%u:%s%sU-%W\\%W,%s:%s%W:%s",
 		     posix_name, uid, gid,
--
2.38.0.rc0.windows.1

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2015-12-17 20:20     ` Corinna Vinschen
@ 2022-09-21 11:58       ` Johannes Schindelin
  2022-10-18 17:02         ` Corinna Vinschen
  0 siblings, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2022-09-21 11:58 UTC (permalink / raw)
  To: cygwin-patches

Hi Corinna,

sorry for the blast from the past, but I am renewing my efforts to
upstream Git for Windows' patches that can be upstreamed.

On Thu, 17 Dec 2015, Corinna Vinschen wrote:

> On Dec 17 19:05, Johannes Schindelin wrote:
> > [...]
> > diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> > index c9b3e09..a5d6270 100644
> > --- a/winsup/cygwin/uinfo.cc
> > +++ b/winsup/cygwin/uinfo.cc
> > [...]
> > +static size_t
> > +fetch_env(LPCWSTR key, char *buf, size_t size)
>            ^^^
>            space
>
> > +{
> > +  WCHAR wbuf[32767];
>
> Ok, there are a couple of problems here.  First, since this buffer
> is a filename buffer, use NT_MAX_PATH from winsup.h as buffer size.
>
> But then again, please avoid allocating 64K buffers on the stack.
> That's what tmp_pathbuf:w_get () is for.

Excellent. I did it exactly as you suggested.

> > +  DWORD max = sizeof wbuf / sizeof *wbuf;
> > +  DWORD len = GetEnvironmentVariableW (key, wbuf, max);
>
> This call to GetEnvironmentVariableW looks gratuitous to me.  Why don't
> you simply call getenv?  It did the entire job already, it avoids the
> requirement for a local buffer, and in case of $HOME it even did the
> Win32->POSIX path conversion.  If there's a really good reason for using
> GetEnvironmentVariableW it begs at least for a longish comment.

My only worry is that `getenv("HOME")` might receive a "Cygwin-ified"
version of the value. That is, `getenv("HOME")` might return something
like `/cygdrive/c/Users/corinna` when we expect it to return
`C:\Users\corinna` instead.

I do not think that the current iteration is resilient against that.

This problem might not be a big issue with Cygwin (I don't think it
automatically converts environment variables that look like paths from
Windows to Unix-style), but it will most likely cause issues with MSYS2
(where we do precisely that with environment variables that look like
paths). Meaning: it will probably take some follow-up work to make this
work correctly, even if it is just to verify that things work when `HOME`
is in Unix-style already while calling into the runtime.

> > +
> > +  if (!len || len >= max)
> > +    return 0;
> > +
> > +  len = sys_wcstombs (buf, size, wbuf, len);
> > +  return len && len < size ? len : 0;
> > +}
> > +
> > +static char *
> > +fetch_home_env (void)
> > +{
> > +  char home[32767];
> > +  size_t max = sizeof home / sizeof *home, len;
> > +
> > +  if (fetch_env (L"HOME", home, max)
> > +      || ((len = fetch_env (L"HOMEDRIVE", home, max))
> > +        && fetch_env (L"HOMEPATH", home + len, max - len))
> > +      || fetch_env (L"USERPROFILE", home, max))
> > +    {
> > +      tmp_pathbuf tp;
> > +      cygwin_conv_path (CCP_WIN_A_TO_POSIX | CCP_ABSOLUTE,
> > +	  home, tp.c_get(), NT_MAX_PATH);
>                        ^^^
>                        space
> > +      return strdup(tp.c_get());
>                      ^^^      ^^^
>                      space......s
>
> Whoa, tp.c_get() twice to access the same space?  That's a dirty trick
> which may puzzle later readers of the code and heavily depends on
> knowing the internals of tmp_pathbuf.  Please use a variable and only
> assign tp.c_get () once.
>
> OTOH, the above's a case for a cygwin_create_path call, rather than
> cygwin_conv_path+strdup.  Also, if there's *really* a good reason to use
> GetEnvironmentVariableW, you should collapse sys_wcstombs+cygwin_conv_path+
> strdup into a single cygwin_create_path (CCP_WIN_W_TO_POSIX, ...).

Right, that `cygwin_create_path()` call nicely avoids all the problems of
my original code.

>
> > [...]
> > @@ -1079,6 +1123,7 @@ cygheap_pwdgrp::get_shell (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
> >  	case NSS_SCHEME_FALLBACK:
> >  	  return NULL;
> >  	case NSS_SCHEME_WINDOWS:
> > +	case NSS_SCHEME_ENV:
> >  	  break;
> >  	case NSS_SCHEME_CYGWIN:
> >  	  if (pldap->fetch_ad_account (sid, false, dnsdomain))
>
> You know that I don't exactly like the "env" idea, but if we implement
> it anyway, wouldn't it make sense to add some kind of $SHELL handling as
> well, for symmetry?

I have decided against that.

The reason: the home directory is a very different thing from the `SHELL`
variable because Windows users _do_ have a home directory even if it is
called differently in Windows speak, while they do not have any POSIX
shell available. There is `COMSPEC`, of course, but it is _not_ a POSIX
shell and cannot be used in place of `SHELL`.

For that reason, I do not believe that we need to do anything about
`SHELL`.

> > [...]
> > @@ -1487,6 +1497,16 @@ of each schema when used with <literal>db_home:</literal>
> >  	      for a detailed description.</listitem>
> >    </varlistentry>
> >    <varlistentry>
> > +    <term><literal>env</literal></term>
> > +    <listitem>Derives the home directory of the current user from the
> > +	      environment variable <literal>HOME</literal> (falling back to
> > +	      <literal>HOMEDRIVE\HOMEPATH</literal> and
> > +	      <literal>USERPROFILE</literal>, in that order).  This is faster
> > +	      than the <term><literal>windows</literal></term> schema at the
> > +	      expense of determining only the current user's home directory
> > +	      correctly.</listitem>
>
> In both case of the documentation it might make sense to add a few words
> along the lines of "This schema is skipped for any other account",
> wouldn't it?

Yes!

(Belated) thank you very much for your review!
Dscho

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account
  2015-12-17 20:49     ` Corinna Vinschen
  2015-12-17 21:02       ` Corinna Vinschen
@ 2022-09-21 12:00       ` Johannes Schindelin
  1 sibling, 0 replies; 25+ messages in thread
From: Johannes Schindelin @ 2022-09-21 12:00 UTC (permalink / raw)
  To: cygwin-patches

Hi Corinna,

On Thu, 17 Dec 2015, Corinna Vinschen wrote:

> On Dec 17 19:05, Johannes Schindelin wrote:
> > We should not blindly set the home directory of the SYSTEM account to
> > /home/SYSTEM, especially not when that value disagrees with what is
> > configured via the `db_home` line in the `/etc/nsswitch.conf` file.
> >
> > This fixes https://github.com/git-for-windows/git/issues/435
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  winsup/cygwin/uinfo.cc | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> > index a5d6270..8c51b82 100644
> > --- a/winsup/cygwin/uinfo.cc
> > +++ b/winsup/cygwin/uinfo.cc
> > @@ -2129,7 +2129,11 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
> >  	 it to a well-known group here. */
> >        if (acc_type == SidTypeUser
> >  	  && (sid_sub_auth_count (sid) <= 3 || sid_id_auth (sid) == 11))
> > -	acc_type = SidTypeWellKnownGroup;
> > +	{
> > +	  acc_type = SidTypeWellKnownGroup;
> > +	  home = cygheap->pg.get_home (pldap, sid, dom, domain, name,
> > +				       fully_qualified_name);
>
> Uhm, that's a bit over the top, isn't it?  It will affect all S-1-5-X
> accounts as well as the S-1-5-11 Windows account SIDs.  Is that really
> what you want?

Yes, it was really what I want because it's about respecting `db_home:
env`, and there _are_ apparently SIDs a user can have that fall into the
category "Microsoft account" where we want that to be respected, too ;-)

> Using pldap here may SEGV in cygheap_pwdgrp::get_home, btw, because
> it may be NULL.  cygheap_pwdgrp::get_home doesn't check pldap for
> validity, it expects a valid pointer.  You could either use cldap, or
> cygheap_pwdgrp::get_home would have to check pldap before using it.
>
> However, either way there's another problem: Independently of the
> configured db_home schemes, you don't want to ask the DC for info on
> these builtin accounts.  The better approach might be to call the
> PUSER_INFO_3 variant of cygheap_pwdgrp::get_home with a NULL ui
> pointer and just check for ui in the NSS_SCHEME_DESC case.  The other
> called functions fetch_windows_home and fetch_from_path both can
> live with both pointers, pldap and ui being NULL.

Excellent, I used the `PUSER_INFO_3` method.

Thank you,
Dscho

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2022-09-21 11:58       ` Johannes Schindelin
@ 2022-10-18 17:02         ` Corinna Vinschen
  2022-10-23 21:04           ` Johannes Schindelin
  0 siblings, 1 reply; 25+ messages in thread
From: Corinna Vinschen @ 2022-10-18 17:02 UTC (permalink / raw)
  To: cygwin-patches

Hi Johannes,

On Sep 21 13:58, Johannes Schindelin wrote:
> Hi Corinna,
> 
> sorry for the blast from the past, but I am renewing my efforts to
> upstream Git for Windows' patches that can be upstreamed.
> 
> On Thu, 17 Dec 2015, Corinna Vinschen wrote:

Well, not even 7 years, so what? :)

> > On Dec 17 19:05, Johannes Schindelin wrote:
> > > +  DWORD max = sizeof wbuf / sizeof *wbuf;
> > > +  DWORD len = GetEnvironmentVariableW (key, wbuf, max);
> >
> > This call to GetEnvironmentVariableW looks gratuitous to me.  Why don't
> > you simply call getenv?  It did the entire job already, it avoids the
> > requirement for a local buffer, and in case of $HOME it even did the
> > Win32->POSIX path conversion.  If there's a really good reason for using
> > GetEnvironmentVariableW it begs at least for a longish comment.
> 
> My only worry is that `getenv("HOME")` might receive a "Cygwin-ified"
> version of the value. That is, `getenv("HOME")` might return something
> like `/cygdrive/c/Users/corinna` when we expect it to return
> `C:\Users\corinna` instead.

Haha, yeah, that's exactly what it does.  Look at environ.cc, search for
conv_envvars.  There's a list of env vars which are converted
automatically.  So getenv ("HOME") already does what you need, you just
have to adapt the code accordingly, i. e.

  if ((home = getenv ("HOME")))
    return strdup (home);
  if (((home_drive = getenv ("HOMEDRIVE")
           [...]
    return (char *) cygwin_create_path (CCP_WIN_A_TO_POSIX, home);

However, on second thought, I wonder if the HOMEDRIVE/HOMEPATH/USERPROFILE
code is really required.  AFAICS, it's just a duplication of the effort
already done in fetch_windows_home(), isn't it?

HOMEDRIVE/HOMEPATH are generated from the DB data returned in
USER_INFO_3 or via ldap anyway, and fetch_windows_home() also falls back
to fetching the user profile path, albeit from the registry.

That means, the results from the "env" method is equivalent to the
"windows" method, just after checking $HOME.  That's a bit of a downer.

Assuming the "env" method would *only* check for $HOME, the user would
have the same result by simply setting nsswitch.conf accordingly:

  home: env windows


Corinna

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2022-10-18 17:02         ` Corinna Vinschen
@ 2022-10-23 21:04           ` Johannes Schindelin
  2022-10-24 11:37             ` Corinna Vinschen
  0 siblings, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2022-10-23 21:04 UTC (permalink / raw)
  To: cygwin-patches

Hi Corinna,

On Tue, 18 Oct 2022, Corinna Vinschen wrote:

> On Sep 21 13:58, Johannes Schindelin wrote:
> > Hi Corinna,
> >
> > sorry for the blast from the past, but I am renewing my efforts to
> > upstream Git for Windows' patches that can be upstreamed.
> >
> > On Thu, 17 Dec 2015, Corinna Vinschen wrote:
>
> Well, not even 7 years, so what? :)

True :-)

> > > On Dec 17 19:05, Johannes Schindelin wrote:
> > > > +  DWORD max = sizeof wbuf / sizeof *wbuf;
> > > > +  DWORD len = GetEnvironmentVariableW (key, wbuf, max);
> > >
> > > This call to GetEnvironmentVariableW looks gratuitous to me.  Why don't
> > > you simply call getenv?  It did the entire job already, it avoids the
> > > requirement for a local buffer, and in case of $HOME it even did the
> > > Win32->POSIX path conversion.  If there's a really good reason for using
> > > GetEnvironmentVariableW it begs at least for a longish comment.
> >
> > My only worry is that `getenv("HOME")` might receive a "Cygwin-ified"
> > version of the value. That is, `getenv("HOME")` might return something
> > like `/cygdrive/c/Users/corinna` when we expect it to return
> > `C:\Users\corinna` instead.
>
> Haha, yeah, that's exactly what it does.  Look at environ.cc, search for
> conv_envvars.  There's a list of env vars which are converted
> automatically.  So getenv ("HOME") already does what you need, you just
> have to adapt the code accordingly, i. e.
>
>   if ((home = getenv ("HOME")))
>     return strdup (home);
>   if (((home_drive = getenv ("HOMEDRIVE")
>            [...]
>     return (char *) cygwin_create_path (CCP_WIN_A_TO_POSIX, home);
>
> However, on second thought, I wonder if the HOMEDRIVE/HOMEPATH/USERPROFILE
> code is really required.  AFAICS, it's just a duplication of the effort
> already done in fetch_windows_home(), isn't it?

You mean the case where _both_ `pldap` and `ui` are `NULL`, i.e. where
`get_user_profile_directory()` is called?

Correct me if I'm wrong, but that does not at all look at the environment
variables, but instead queries the registry.

And _if_ we want to do that (which I would rather want to avoid, for
simplicity and speed), shouldn't we call the
`get_user_profile_directory()` function directly instead of going through
`fetch_windows_home()`?

But if you meant to still have a non-`NULL` `pldap`, the results are
definitely not the same, not only because users can easily modify their
environment variables while they cannot easily modify what is retrieved
from the DB: [see below].

> HOMEDRIVE/HOMEPATH are generated from the DB data returned in
> USER_INFO_3 or via ldap anyway, and fetch_windows_home() also falls back
> to fetching the user profile path, albeit from the registry.

The big difference of using ldap is that retrieving the environment
variable is instantaneous whereas there is a multi-second delay if the
domain controller is temporarily unreachable.

> That means, the results from the "env" method is equivalent to the
> "windows" method, just after checking $HOME.  That's a bit of a downer.
>
> Assuming the "env" method would *only* check for $HOME, the user would
> have the same result by simply setting nsswitch.conf accordingly:
>
>   home: env windows

Except when the domain controller is (temporarily) unreachable, e.g. when
sitting in a train with poor or no internet connection. Then that latter
approach would have the "benefit" of having to wait 10-15 seconds before
the network call says "nope".

This particular issue has hit enough Git for Windows users that I found
myself being forced to implement these patches and run with them for the
past seven years.

Given the scenario of an unreachable domain controller, I hope you agree
that the `env` support added in the proposed patches _has_ merit.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2022-10-23 21:04           ` Johannes Schindelin
@ 2022-10-24 11:37             ` Corinna Vinschen
  2022-11-10 15:16               ` Johannes Schindelin
  0 siblings, 1 reply; 25+ messages in thread
From: Corinna Vinschen @ 2022-10-24 11:37 UTC (permalink / raw)
  To: cygwin-patches

On Oct 23 23:04, Johannes Schindelin wrote:
> On Tue, 18 Oct 2022, Corinna Vinschen wrote:
> [...]
> > That means, the results from the "env" method is equivalent to the
> > "windows" method, just after checking $HOME.  That's a bit of a downer.
> >
> > Assuming the "env" method would *only* check for $HOME, the user would
> > have the same result by simply setting nsswitch.conf accordingly:
> >
> >   home: env windows
> 
> Except when the domain controller is (temporarily) unreachable, e.g. when
> sitting in a train with poor or no internet connection. Then that latter
> approach would have the "benefit" of having to wait 10-15 seconds before
> the network call says "nope".
> 
> This particular issue has hit enough Git for Windows users that I found
> myself being forced to implement these patches and run with them for the
> past seven years.
> 
> Given the scenario of an unreachable domain controller, I hope you agree
> that the `env` support added in the proposed patches _has_ merit.

Yes, I don't doubt an `env' method checking for $HOME even a bit.

I'm just not sure as far as HOMEDRIVE/HOMEPATH/USERPROFILE are
concerned.  Those vars should be left alone, but we can't control that,
so reading them from genuine sources is preferred.

Sure, the downside in terms of the LDAP server is clear to me

So I guess it's ok to allow the env method to read the values of those
vars from the env.  I would just feel better if we urge the
user to set $HOME and read that exclusively.


Corinna

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2022-10-24 11:37             ` Corinna Vinschen
@ 2022-11-10 15:16               ` Johannes Schindelin
  2022-11-10 15:22                 ` Corinna Vinschen
  0 siblings, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2022-11-10 15:16 UTC (permalink / raw)
  To: cygwin-patches

Hi Corinna,

On Mon, 24 Oct 2022, Corinna Vinschen wrote:

> On Oct 23 23:04, Johannes Schindelin wrote:
> > On Tue, 18 Oct 2022, Corinna Vinschen wrote:
> > [...]
> > > That means, the results from the "env" method is equivalent to the
> > > "windows" method, just after checking $HOME.  That's a bit of a downer.
> > >
> > > Assuming the "env" method would *only* check for $HOME, the user would
> > > have the same result by simply setting nsswitch.conf accordingly:
> > >
> > >   home: env windows
> >
> > Except when the domain controller is (temporarily) unreachable, e.g. when
> > sitting in a train with poor or no internet connection. Then that latter
> > approach would have the "benefit" of having to wait 10-15 seconds before
> > the network call says "nope".
> >
> > This particular issue has hit enough Git for Windows users that I found
> > myself being forced to implement these patches and run with them for the
> > past seven years.
> >
> > Given the scenario of an unreachable domain controller, I hope you agree
> > that the `env` support added in the proposed patches _has_ merit.
>
> Yes, I don't doubt an `env' method checking for $HOME even a bit.

Cool!

> I'm just not sure as far as HOMEDRIVE/HOMEPATH/USERPROFILE are
> concerned.  Those vars should be left alone, but we can't control that,
> so reading them from genuine sources is preferred.

I do not recall the exact reasons because it has been a good while since I
worked on these patches. But I do remember that we had to have a fall-back
for the many scenarios in Git for Windows where `HOME` is not even set,
and we specifically had to add HOMEDRIVE/HOMEPATH handling because
USERPROFILE alone would lead to problems (IIRC there were plenty of
corporate setups where USERPROFILE pointed to a potentially-disconnected
network drive).

> Sure, the downside in terms of the LDAP server is clear to me
>
> So I guess it's ok to allow the env method to read the values of those
> vars from the env.  I would just feel better if we urge the
> user to set $HOME and read that exclusively.

I would feel better about that, too, if it was practical.

But I cannot ask millions of Git for Windows users to please go ahead and
first configure their `HOME` variable correctly, it took much less time to
implement the patch we're discussing than asking all users individually
;-)

And since there is nothing specific about Git for Windows here, I expect
Cygwin users to benefit from this feature, too.

With this context in mind, I would like to ask to integrate the patch
as-is, including the HOMEDRIVE/HOMEPATH and USERPROFILE fall-backs.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2022-11-10 15:16               ` Johannes Schindelin
@ 2022-11-10 15:22                 ` Corinna Vinschen
  2022-11-18  8:18                   ` Johannes Schindelin
  0 siblings, 1 reply; 25+ messages in thread
From: Corinna Vinschen @ 2022-11-10 15:22 UTC (permalink / raw)
  To: cygwin-patches

On Nov 10 16:16, Johannes Schindelin wrote:
> Hi Corinna,
> 
> On Mon, 24 Oct 2022, Corinna Vinschen wrote:
> 
> > On Oct 23 23:04, Johannes Schindelin wrote:
> > > On Tue, 18 Oct 2022, Corinna Vinschen wrote:
> > > [...]
> > > > That means, the results from the "env" method is equivalent to the
> > > > "windows" method, just after checking $HOME.  That's a bit of a downer.
> > > >
> > > > Assuming the "env" method would *only* check for $HOME, the user would
> > > > have the same result by simply setting nsswitch.conf accordingly:
> > > >
> > > >   home: env windows
> > >
> > > Except when the domain controller is (temporarily) unreachable, e.g. when
> > > sitting in a train with poor or no internet connection. Then that latter
> > > approach would have the "benefit" of having to wait 10-15 seconds before
> > > the network call says "nope".
> > >
> > > This particular issue has hit enough Git for Windows users that I found
> > > myself being forced to implement these patches and run with them for the
> > > past seven years.
> > >
> > > Given the scenario of an unreachable domain controller, I hope you agree
> > > that the `env` support added in the proposed patches _has_ merit.
> >
> > Yes, I don't doubt an `env' method checking for $HOME even a bit.
> 
> Cool!
> 
> > I'm just not sure as far as HOMEDRIVE/HOMEPATH/USERPROFILE are
> > concerned.  Those vars should be left alone, but we can't control that,
> > so reading them from genuine sources is preferred.
> 
> I do not recall the exact reasons because it has been a good while since I
> worked on these patches. But I do remember that we had to have a fall-back
> for the many scenarios in Git for Windows where `HOME` is not even set,
> and we specifically had to add HOMEDRIVE/HOMEPATH handling because
> USERPROFILE alone would lead to problems (IIRC there were plenty of
> corporate setups where USERPROFILE pointed to a potentially-disconnected
> network drive).
> 
> > Sure, the downside in terms of the LDAP server is clear to me
> >
> > So I guess it's ok to allow the env method to read the values of those
> > vars from the env.  I would just feel better if we urge the
> > user to set $HOME and read that exclusively.
> 
> I would feel better about that, too, if it was practical.
> 
> But I cannot ask millions of Git for Windows users to please go ahead and
> first configure their `HOME` variable correctly, it took much less time to
> implement the patch we're discussing than asking all users individually
> ;-)
> 
> And since there is nothing specific about Git for Windows here, I expect
> Cygwin users to benefit from this feature, too.
> 
> With this context in mind, I would like to ask to integrate the patch
> as-is, including the HOMEDRIVE/HOMEPATH and USERPROFILE fall-backs.

Can't do that, sorry.  Two replies before I sent a necessary change,
which needs inclusion first.


Thanks,
Corinna

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2022-11-10 15:22                 ` Corinna Vinschen
@ 2022-11-18  8:18                   ` Johannes Schindelin
  2022-11-21 11:41                     ` Corinna Vinschen
  0 siblings, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2022-11-18  8:18 UTC (permalink / raw)
  To: Corinna Vinschen; +Cc: cygwin-patches

Hi Corinna,

On Thu, 10 Nov 2022, Corinna Vinschen wrote:

> On Nov 10 16:16, Johannes Schindelin wrote:
>
> > On Mon, 24 Oct 2022, Corinna Vinschen wrote:
> >
> > > On Oct 23 23:04, Johannes Schindelin wrote:
> > > > On Tue, 18 Oct 2022, Corinna Vinschen wrote:
> > > > [...]
> > > > > That means, the results from the "env" method is equivalent to the
> > > > > "windows" method, just after checking $HOME.  That's a bit of a downer.
> > > > >
> > > > > Assuming the "env" method would *only* check for $HOME, the user would
> > > > > have the same result by simply setting nsswitch.conf accordingly:
> > > > >
> > > > >   home: env windows
> > > >
> > > > Except when the domain controller is (temporarily) unreachable, e.g. when
> > > > sitting in a train with poor or no internet connection. Then that latter
> > > > approach would have the "benefit" of having to wait 10-15 seconds before
> > > > the network call says "nope".
> > > >
> > > > This particular issue has hit enough Git for Windows users that I found
> > > > myself being forced to implement these patches and run with them for the
> > > > past seven years.
> > > >
> > > > Given the scenario of an unreachable domain controller, I hope you agree
> > > > that the `env` support added in the proposed patches _has_ merit.
> > >
> > > Yes, I don't doubt an `env' method checking for $HOME even a bit.
> >
> > Cool!
> >
> > > I'm just not sure as far as HOMEDRIVE/HOMEPATH/USERPROFILE are
> > > concerned.  Those vars should be left alone, but we can't control that,
> > > so reading them from genuine sources is preferred.
> >
> > I do not recall the exact reasons because it has been a good while since I
> > worked on these patches. But I do remember that we had to have a fall-back
> > for the many scenarios in Git for Windows where `HOME` is not even set,
> > and we specifically had to add HOMEDRIVE/HOMEPATH handling because
> > USERPROFILE alone would lead to problems (IIRC there were plenty of
> > corporate setups where USERPROFILE pointed to a potentially-disconnected
> > network drive).
> >
> > > Sure, the downside in terms of the LDAP server is clear to me
> > >
> > > So I guess it's ok to allow the env method to read the values of those
> > > vars from the env.  I would just feel better if we urge the
> > > user to set $HOME and read that exclusively.
> >
> > I would feel better about that, too, if it was practical.
> >
> > But I cannot ask millions of Git for Windows users to please go ahead and
> > first configure their `HOME` variable correctly, it took much less time to
> > implement the patch we're discussing than asking all users individually
> > ;-)
> >
> > And since there is nothing specific about Git for Windows here, I expect
> > Cygwin users to benefit from this feature, too.
> >
> > With this context in mind, I would like to ask to integrate the patch
> > as-is, including the HOMEDRIVE/HOMEPATH and USERPROFILE fall-backs.
>
> Can't do that, sorry.  Two replies before I sent a necessary change,
> which needs inclusion first.

I am a bit confused. Do you need anything from me to move this along, i.e.
are those two replies you mention some emails I failed to address yet?

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable
  2022-11-18  8:18                   ` Johannes Schindelin
@ 2022-11-21 11:41                     ` Corinna Vinschen
  0 siblings, 0 replies; 25+ messages in thread
From: Corinna Vinschen @ 2022-11-21 11:41 UTC (permalink / raw)
  To: cygwin-patches

On Nov 18 09:18, Johannes Schindelin wrote:
> Hi Corinna,
> 
> On Thu, 10 Nov 2022, Corinna Vinschen wrote:
> > On Nov 10 16:16, Johannes Schindelin wrote:
> > > With this context in mind, I would like to ask to integrate the patch
> > > as-is, including the HOMEDRIVE/HOMEPATH and USERPROFILE fall-backs.
> >
> > Can't do that, sorry.  Two replies before I sent a necessary change,
> > which needs inclusion first.
> 
> I am a bit confused. Do you need anything from me to move this along, i.e.
> are those two replies you mention some emails I failed to address yet?

I didn't mean two different replies, but my second-last reply before
that one, i. e.
https://cygwin.com/pipermail/cygwin-patches/2022q4/012025.html

Sorry if that wasn't clear.  Basically your handling of $HOME was
wrong and I suggested a fix.


Corinna

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2022-11-21 11:41 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-16 13:06 [PATCH] Allow overriding the home directory via the HOME variable Johannes Schindelin
2015-10-21 18:32 ` Corinna Vinschen
2015-10-22 15:38   ` Johannes Schindelin
2015-10-23  9:10     ` Corinna Vinschen
2015-10-23  9:41       ` Corinna Vinschen
2015-10-23 12:00         ` Johannes Schindelin
2015-12-17 18:05 ` [PATCH v2 0/2] Support deriving the current user's home directory via HOME Johannes Schindelin
2015-12-17 18:05   ` [PATCH v2 2/2] Respect `db_home` setting even for the SYSTEM account Johannes Schindelin
2015-12-17 20:49     ` Corinna Vinschen
2015-12-17 21:02       ` Corinna Vinschen
2022-09-21 12:00       ` Johannes Schindelin
2015-12-17 18:05   ` [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable Johannes Schindelin
2015-12-17 20:20     ` Corinna Vinschen
2022-09-21 11:58       ` Johannes Schindelin
2022-10-18 17:02         ` Corinna Vinschen
2022-10-23 21:04           ` Johannes Schindelin
2022-10-24 11:37             ` Corinna Vinschen
2022-11-10 15:16               ` Johannes Schindelin
2022-11-10 15:22                 ` Corinna Vinschen
2022-11-18  8:18                   ` Johannes Schindelin
2022-11-21 11:41                     ` Corinna Vinschen
2022-09-21 11:51   ` [PATCH v3 0/3] Support deriving the current user's home directory via HOME Johannes Schindelin
2022-09-21 11:51     ` [PATCH v3 1/3] Allow deriving the current user's home directory via the HOME variable Johannes Schindelin
2022-09-21 11:52     ` [PATCH v3 2/3] Respect `db_home` setting even for SYSTEM/Microsoft accounts Johannes Schindelin
2022-09-21 11:52     ` [PATCH v3 3/3] Respect `db_home: env` even when no uid can be determined Johannes Schindelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).