windows: Read the PATH env var of the child #1337

alexcrichton · 2014-06-24T23:01:19Z

The unix and windows process implementations diverge in their behavior
when dealing with subprocesses that are spawned with a relative path.
With unix the child's PATH environment variable is read, whereas
with windows the parent's environment variable is read.

This commit brings the two implementation in line with respect to
their behavior of reading PATH by having both read the child's PATH
environment variable. This involves looking into the user-provided
environment on windows and extracting the PATH variable specifically
so it can be inspected later on.

I'll cc #122 here as well, although I ended up discovering that after
writing this. It appears that there's more to this than I originally
thought, but I figured I'd at least put this out there!

saghul · 2014-06-25T08:42:20Z

Thanks Alex! I'll take a look. It will take a bit since I'm a bit flooded right now, but I'll get to it (if nobody does beforehand).

saghul · 2014-07-30T07:16:41Z

@piscisaureus this is a bit over my head, can you help please? :-)

piscisaureus · 2014-07-31T11:19:28Z

@saghul

No time today but I will try to find a spare hour or so.
It's not super difficult: when the user specifies the environment for a child process (via the uv_process_options struct), we should use the PATH in that environment, and not the PATH of the parent process.

The affected code is here:

libuv/src/win/process.c

Lines 878 to 899 in c11a598

    
           /* Get PATH environment variable. */ 
        
           { 
        
             DWORD path_len, r; 
        
             path_len = GetEnvironmentVariableW(L"PATH", NULL, 0); 
        
             if (path_len == 0) { 
        
               err = GetLastError(); 
        
               goto done; 
        
             } 
        
             path = (WCHAR*) malloc(path_len * sizeof(WCHAR)); 
        
             if (path == NULL) { 
        
               err = ERROR_OUTOFMEMORY; 
        
               goto done; 
        
             } 
        
             r = GetEnvironmentVariableW(L"PATH", path, path_len); 
        
             if (r == 0 || r >= path_len) { 
        
               err = GetLastError(); 
        
               goto done; 
        
             } 
        
           }

.

In pseudocode it should look like:

if (environment specified in process_options) {
  // get path from options;
  // if not found set path to the empty string;
} else {
  // use path from parent process  
  path = GetEnvironmentVariableW('path');
}

saghul · 2014-07-31T11:25:22Z

@piscisaureus Thanks! I'll also go through it.

saghul · 2014-08-05T08:15:01Z

Hi @alexcrichton, sorry for the delay on this. Unfortunately your patch no longer applies because make_program_env was heavily modified to require extra env variables and sort them in the right order. I think it would be best so search for "PATH=" in the provessed env rather than doing it as part of make_program_env, since it's already quite complex.

Can you rebase and work on a patch that applies to current master?

alexcrichton · 2014-08-05T14:51:45Z

Sure! I've rebased and tested on my local mingw installation, so hopefully it's working!

saghul · 2014-08-05T15:08:51Z

src/win/process.c

+ * If found, `*path` and `*path_len` will be the value of PATH and the lenth,
+ * respectively, and 1 will be returned. If not found, then 0 will be returned.
+ */
+static int find_path(WCHAR *env, WCHAR **path, DWORD *path_len) {


Can you make this set path to NULL and path_len to 0 in case it doesn't find it instead? You can also make it return *path to make the if check nicer.

saghul · 2014-08-05T15:13:31Z

Sure! I've rebased and tested on my local mingw installation, so hopefully it's working!

Nice, thanks! Left a few more coments, nothing big though, the approach seems fine :-)

The unix and windows process implementations diverge in their behavior when dealing with subprocesses that are spawned with a relative path. With unix the *child's* PATH environment variable is read, whereas with windows the *parent's* environment variable is read. This commit brings the two implementation in line with respect to their behavior of reading PATH by having both read the *child's* PATH environment variable. This involves looking into the user-provided environment on windows and extracting the PATH variable specifically so it can be inspected later on.

alexcrichton · 2014-08-05T15:17:57Z

Thanks for taking a look! I've updated the patch.

saghul · 2014-08-05T15:22:23Z

src/win/process.c

@@ -1131,7 +1147,7 @@ int uv_spawn(uv_loop_t* loop,
  free(arguments);
  free(cwd);
  free(env);
-  free(path);
+  free(alloc_path);


Is this correct? We set path to alloc_path, so we should be freeing that one, right?

alloc_path is the variable storing the allocation, and path is just a pointer to something which is the actual path value (may or may not be an allocation). If PATH is found in env then path doesn't need to be free'd (it's in the middle of an allocation).

Ah, right. LGTM then.

saghul · 2014-08-05T15:24:01Z

Left a couple more comments, we are almost there :-)

alexcrichton · 2014-08-05T23:00:57Z

@saghul, looks ok?

saghul · 2014-08-05T23:01:36Z

@alexcrichton yeah, will merge now! 👍

saghul · 2014-08-05T23:28:07Z

I just ran the tests on my Windows 7 machine and uv_spawn fails with ENOENT for me. Does the test pass for you?

alexcrichton · 2014-08-05T23:33:23Z

Running the tests locally causes the fs_file_loop test to fail, but other than that all other tests pass. What does the failure look like for you?

saghul · 2014-08-05T23:39:44Z

How are you running your tests? I'm running vcbuild.bat test in a PowerShell prompt. Test fails as follows:

[%  72|+ 165|-   2|T   0|S   0]: spawn_reads_child_path
`spawn_reads_child_path` failed: exit code 3
Output from process `spawn_reads_child_path`:
Assertion failed in test\test-spawn.c on line 1318: r == 0

I printed the error and it's ENOENT, so I assume it comes from here: https://github.com/joyent/libuv/blob/master/src/win/process.c#L1018

alexcrichton · 2014-08-05T23:44:46Z

Ah I may be running the tests incorrectly then. I'm using mingw and I don't know how to run the tests otherwise so it looks something like this:

$ gcc -Iinclude -Itest $(ls tests/*.c | grep -v unix | grep -v benchmarks) -L. -luv -lws2_32 -lpsapi -liphlpapi
$ ./a.exe

Could you add a print to search_path to see what path is being searched? I could also try to install power shell and friends to reproduce if you're busy!

saghul · 2014-08-05T23:52:46Z

That's an unusual way to run tests :-) I just ran them on a MSYS2/MinGW environment (built it with autotools), and the failure is the same. cmd.exe gives the same error :-S

I'll add some prints.

saghul · 2014-08-06T00:04:18Z

Hum, here are the paths being searched:

-XX C:\Users\saghul\src\libuv\run-tests.com
-XX C:\Users\saghul\src\libuv\run-tests.exe
-XX C:\Users\saghul\src\libuv\%SystemRoot%\system32\WindowsPowerShell\v1.0\run-tests.com
-XX C:\Users\saghul\src\libuv\%SystemRoot%\system32\WindowsPowerShell\v1.0\run-tests.exe
-XX C:\Windows\system32\run-tests.com
-XX C:\Windows\system32\run-tests.exe
-XX C:\Windows\run-tests.com
-XX C:\Windows\run-tests.exe
-XX C:\Windows\System32\Wbem\run-tests.com
-XX C:\Windows\System32\Wbem\run-tests.exe
-XX C:\Windows\System32\WindowsPowerShell\v1.0\run-tests.com
-XX C:\Windows\System32\WindowsPowerShell\v1.0\run-tests.exe
-XX C:\Python27\run-tests.com
-XX C:\Python27\run-tests.exe
-XX C:\Python27\Scripts\run-tests.com
-XX C:\Python27\Scripts\run-tests.exe
-XX C:\Program Files\Microsoft Windows Performance Toolkit\run-tests.com
-XX C:\Program Files\Microsoft Windows Performance Toolkit\run-tests.exe
-XX C:\ProgramData\chocolatey\bin\run-tests.com
-XX C:\ProgramData\chocolatey\bin\run-tests.exe

Of course it fails, the path where the exe is located is C:\Users\saghul\src\libuv\Debug. Maybe the bug is in search_path...

saghul · 2014-08-06T00:13:24Z

Got it! The PATH wasn't actually properly found:

for (; env != NULL && *env != 0; env += wcslen(env)) {

vs

for (; env != NULL && *env != 0; env += wcslen(env) + 1) {

We also need to skip the null byte at the end of each env entry.

I'll amend your patch and merge.

alexcrichton · 2014-08-06T00:14:01Z

Oh whoops, thanks @saghul for tracking that down!

saghul · 2014-08-06T00:17:15Z

Landed in c7e4b31, thanks Alex!

PS: Nice PR number ;-)

alexcrichton mentioned this pull request Jun 24, 2014

Process spawning via PATH is different on unix than it is on windows rust-lang/rust#15149

Closed

saghul added this to the v0.12 milestone Jul 19, 2014

saghul reviewed Aug 5, 2014
View reviewed changes

saghul closed this Aug 6, 2014

alexcrichton deleted the read-child-path branch August 6, 2014 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows: Read the PATH env var of the child #1337

windows: Read the PATH env var of the child #1337

alexcrichton commented Jun 24, 2014

saghul commented Jun 25, 2014

saghul commented Jul 30, 2014

piscisaureus commented Jul 31, 2014

saghul commented Jul 31, 2014

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul Aug 5, 2014

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul Aug 5, 2014

alexcrichton Aug 5, 2014

saghul Aug 5, 2014

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul commented Aug 5, 2014

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul commented Aug 5, 2014

saghul commented Aug 6, 2014

saghul commented Aug 6, 2014

alexcrichton commented Aug 6, 2014

saghul commented Aug 6, 2014

windows: Read the PATH env var of the child #1337

windows: Read the PATH env var of the child #1337

Conversation

alexcrichton commented Jun 24, 2014

saghul commented Jun 25, 2014

saghul commented Jul 30, 2014

piscisaureus commented Jul 31, 2014

saghul commented Jul 31, 2014

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul Aug 5, 2014

Choose a reason for hiding this comment

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul Aug 5, 2014

Choose a reason for hiding this comment

alexcrichton Aug 5, 2014

Choose a reason for hiding this comment

saghul Aug 5, 2014

Choose a reason for hiding this comment

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul commented Aug 5, 2014

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul commented Aug 5, 2014

alexcrichton commented Aug 5, 2014

saghul commented Aug 5, 2014

saghul commented Aug 6, 2014

saghul commented Aug 6, 2014

alexcrichton commented Aug 6, 2014

saghul commented Aug 6, 2014