PowerShell doesn't start if env var HOME is not defined #3437

SteveL-MSFT · 2017-03-27T22:47:37Z

In some environments (like Puppet), the HOME env var is not defined and this causes PowerShell to fail to start as it assumes HOME is available and tries to end up concatenating $null with a string. Fix is to return an empty string instead of $null. Side effect is that some working directories like .cache get created in the current working directory instead of HOME (which doesn't exist in this case) which seems acceptable.

Addresses #1794

iSazonov · 2017-03-28T04:52:04Z

Can we use something like %temp% instead of the null?
http://stackoverflow.com/questions/31068/how-do-i-find-the-temp-directory-in-linux
http://unix.stackexchange.com/questions/174817/finding-the-correct-tmp-dir-on-multiple-platforms

iSazonov · 2017-03-28T05:04:06Z

We can use Path.GetTempPath

SteveL-MSFT · 2017-03-28T16:22:13Z

I'll update it to use GetTempPath()

lzybkr · 2017-03-28T17:56:57Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+        /// <summary>
+        /// Get a temporary directory to use, needs to be unique to avoid collision
+        /// </summary>
+        public static string GetTemporaryDirectory()


Does this need to be public?

And shouldn't it return the same directory every time it's called?

making it internal. yeah, it should return the same directory per process

lzybkr · 2017-03-28T17:58:39Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+            string tempDir = "";
+            do
+            {
+                tempDir = Path.Combine(Path.GetTempPath(),System.Guid.NewGuid().ToString());


It doesn't really matter here because the loop should only run once, but as a matter of style, you should move calls that always (or should always) return the same thing (like Path.GetTempPath) outside of the loop.

lzybkr · 2017-03-28T17:59:21Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+
+            try
+            {
+                Directory.CreateDirectory(tempDir);


There is still a race here - extremely unlikely, but another thread/process could create the same directory you're trying to create.

CreateDirectory will return tempDir if that directory already exists

iSazonov · 2017-03-28T18:05:41Z

src/System.Management.Automation/CoreCLR/CorePsExtensions.cs

@@ -1044,6 +1044,11 @@ private static string InternalGetFolderPath(SpecialFolder folder)
            string folderPath = null;

 #if UNIX
+            string envHome = System.Environment.GetEnvironmentVariable("HOME");


Should we protect Windows too?

Looking at the code where "USERPROFILE" env var is used, it appears to handle the case where it's null already

I see.
Closed.

iSazonov · 2017-03-28T18:07:19Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+        /// <summary>
+        /// Get a temporary directory to use, needs to be unique to avoid collision
+        /// </summary>
+        public static string GetTemporaryDirectory()


I open Issue to enhance New-TemporaryFile cmdlet to return a temporary directory.
So maybe we move the code to PSUtils.cs?

iSazonov · 2017-03-28T18:08:55Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+            {
+                tempDir = Path.Combine(Path.GetTempPath(),System.Guid.NewGuid().ToString());
+            }
+            while (Directory.Exists(tempDir));


Infinite loop can happen.

not sure how we can be in an infinite loop realistically here

iSazonov · 2017-03-28T18:09:46Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

+            }
+            catch (UnauthorizedAccessException)
+            {
+                return "";  // will become current working directory


Maybe string.Empty?

iSazonov · 2017-03-29T05:39:18Z

src/System.Management.Automation/CoreCLR/CorePsExtensions.cs

                    break;
                case SpecialFolder.LocalApplicationData:
-                    folderPath = System.IO.Path.Combine(System.Environment.GetEnvironmentVariable("HOME"), ".config");
+                    folderPath = System.IO.Path.Combine(envHome, ".config");
                    if (!System.IO.Directory.Exists(folderPath)) { System.IO.Directory.CreateDirectory(folderPath); }


In the worst case, here we can get UnauthorizedAccessException.

Will wrap in try..catch

iSazonov · 2017-03-29T05:43:08Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

@@ -204,6 +206,20 @@ internal static class CommonEnvVariableNames
 #endif
        }

+        /// <summary>
+        /// Get a temporary directory to use, needs to be unique to avoid collision


I believe the comment not clear enough (unique for current process?).

Agree, updated

iSazonov · 2017-03-29T05:51:19Z

src/System.Management.Automation/utils/PsUtils.cs

+            {
+                tempDir = Path.Combine(tempPath,System.Guid.NewGuid().ToString());
+            }
+            while (Directory.Exists(tempDir));


Maybe remove the cycle? If we believe that it cannot be infinite that means we believe that NewGuid is always globally unique and then the cycle is generally not needed.

It seems like the risk of an infinite loop is slightly lower than the risk of NewGuid() not being unique although realistically, I expect both to be practically not a real issue but having the loop seems very slightly safer.

iSazonov · 2017-03-29T17:08:19Z

src/System.Management.Automation/CoreCLR/CorePsExtensions.cs

+                        }
+                        catch (UnauthorizedAccessException)
+                        {
+                            // some scenarios run as accounts without filesystem access


Oh, sorry, I again don't understand the comment for the first time. Could you make it more clear?

lzybkr · 2017-03-29T17:47:39Z

src/System.Management.Automation/utils/PsUtils.cs

+            string tempPath = Path.GetTempPath();
+            do
+            {
+                tempDir = Path.Combine(tempPath,System.Guid.NewGuid().ToString());


Assuming this scenario is common (no HOME), we'll eventually create a ton of these directories.

I think we should either:

Use a reproducible name

Remove the directory before the process exits

Don't know how common this will be, but cleaning up makes sense. Putting it in ConsoleHost.Dispose()?

If this is a common scenario, then perhaps we should require to specify a temporary directory using one of the possible ways?

@iSazonov although not discoverable, for advanced uses you can define XDG_CONFIG_HOME, XDG_CACHE_HOME, and XDG_DATA_HOME env vars which PowerShell will use (on non-Windows). This was really only to support simple cases (like Puppet) to not put a barrier for adoption of PowerShell

I meant that maybe we need to specify in the documentation that for proper PowerShell operation, users need to define (enumeration here) variables.

Created doc bug MicrosoftDocs/PowerShell-Docs#1126

I don't think users should be required to create those env vars, we should just work in most cases

iSazonov · 2017-03-30T03:38:51Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

@@ -195,7 +197,7 @@ internal static bool IsInbox
        /// Some common environment variables used in PS have different
        /// names in different OS platforms
        /// </summary>
-        internal static class CommonEnvVariableNames
+        public static class CommonEnvVariableNames


should it really be "public"?

I was thinking it would be useful for cmdlet authors rather than having to rely on HOME for Unix and USERPROFILE for Windows

Actually, perhaps we should keep it internal for now until https://github.com/PowerShell/PowerShell-RFC/blob/master/1-Draft/RFC0019-PowerShell-Core-Interop-Module.md gets resolved

It's just a constant - makes no sense.
We would discuss this in the topic about creating platform unified scripts to get common solution and best practices. But we have not yet created such a discussion.

You said the first :-)

iSazonov · 2017-03-30T03:52:36Z

src/System.Management.Automation/utils/PsUtils.cs

+            string tempPath = Path.GetTempPath();
+            do
+            {
+                tempDir = Path.Combine(tempPath,System.Guid.NewGuid().ToString());


I meant that maybe we need to specify in the documentation that for proper PowerShell operation, users need to define (enumeration here) variables.

SteveL-MSFT · 2017-03-31T18:15:43Z

@iSazonov @lzybkr any other feedback?

iSazonov · 2017-03-31T18:18:14Z

LGTM.

SteveL-MSFT · 2017-04-01T03:21:44Z

@iSazonov can you mark your review as Approved? thanks

iSazonov · 2017-04-01T07:56:20Z

Approved.

lzybkr

Looks good except there is no cleanup.

We don't really have an ideal place for engine wide cleanup. ConsoleHost.Dispose isn't it - that only applies to the console host.

One possibility is when closing the last runspace (LocalRunspace.Dispose or something like that.) This is somewhat clean because we track all of the runspaces for debugging, but it does require extra care because a host may create new runspaces after you've deleted the directory.

SteveL-MSFT · 2017-04-02T01:51:18Z

@lzybkr clean up added

iSazonov · 2017-04-02T07:46:38Z

I'd not worried much about cleaning up temporary files and directories because the implication is that it's "trash", which can be removed by the user or by the system at any time. For example, Windows has Disk CleanUp Utility (I run it regularly on users computer by SCCM, GPO and scheduled tasks) and a group policy to create a temporary directory on the user session (%temp% is created on logon and removed on logoff).
Added clean-up is 👍

lzybkr · 2017-04-03T18:09:09Z

Cleanup is important because shell processes are sometimes created frequently, and degrading system performance due to too many directories in TMP is not something we want to do. Not everybody cleans up TMP, or maybe not often enough at any rate.

lzybkr · 2017-04-03T18:12:01Z

@SteveL-MSFT - I've approved the code, but would you mind squashing and improving the commit message?

SteveL-MSFT · 2017-04-03T18:59:17Z

@lzybkr I'll take care of it next week, at Whistler this week without my laptop

SteveL-MSFT · 2017-04-09T03:35:46Z

@lzybkr better?

used does not have a home directory. Updated PowerShell to use a process specific temporary directory if HOME, CONFIG, CACHE, and DATA directories are not availble. Temporary directory is cleaned up when last runspace is disposed.

SteveL-MSFT requested a review from lzybkr March 27, 2017 22:47

msftclas added the cla-not-required label Mar 27, 2017

SteveL-MSFT requested a review from iSazonov March 28, 2017 16:47

lzybkr reviewed Mar 28, 2017

View reviewed changes

iSazonov reviewed Mar 28, 2017

View reviewed changes

SteveL-MSFT assigned lzybkr Mar 29, 2017

iSazonov reviewed Mar 29, 2017

View reviewed changes

SteveL-MSFT force-pushed the nohome branch from 2e9690d to 680ad03 Compare March 29, 2017 17:29

lzybkr reviewed Mar 29, 2017

View reviewed changes

iSazonov reviewed Mar 30, 2017

View reviewed changes

SteveL-MSFT force-pushed the nohome branch from 680ad03 to 1408dc7 Compare March 30, 2017 17:44

iSazonov approved these changes Apr 1, 2017

View reviewed changes

lzybkr suggested changes Apr 1, 2017

View reviewed changes

SteveL-MSFT force-pushed the nohome branch from d310b41 to c161f7c Compare April 2, 2017 00:33

lzybkr approved these changes Apr 3, 2017

View reviewed changes

SteveL-MSFT force-pushed the nohome branch from c161f7c to 8a861c0 Compare April 9, 2017 03:35

SteveL-MSFT force-pushed the nohome branch from 8a861c0 to 28f725e Compare April 9, 2017 03:51

lzybkr merged commit 753b196 into PowerShell:master Apr 9, 2017

SteveL-MSFT deleted the nohome branch June 19, 2017 21:16

PowerShell doesn't start if env var HOME is not defined #3437

PowerShell doesn't start if env var HOME is not defined #3437

Uh oh!

Conversation

SteveL-MSFT commented Mar 27, 2017

Uh oh!

iSazonov commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 28, 2017

Uh oh!

SteveL-MSFT commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

iSazonov commented Mar 28, 2017 •

edited

Loading