Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions src/win/process.c
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,20 @@ int make_program_env(char* env_block[], WCHAR** dst_ptr) {
return 0;
}

/*
* Attempt to find the value of the PATH environment variable in the child's
* preprocessed environment.
*
* If found, a pointer into `env` is returned. If not found, NULL is returned.
*/
static WCHAR* find_path(WCHAR *env) {
for (; env != NULL && *env != 0; env += wcslen(env)) {
if (wcsncmp(env, L"PATH=", 5) == 0)
return &env[5];
}

return NULL;
}

/*
* Called on Windows thread-pool thread to indicate that
Expand Down Expand Up @@ -910,7 +924,7 @@ int uv_spawn(uv_loop_t* loop,
const uv_process_options_t* options) {
int i;
int err = 0;
WCHAR* path = NULL;
WCHAR* path = NULL, *alloc_path = NULL;
BOOL result;
WCHAR* application_path = NULL, *application = NULL, *arguments = NULL,
*env = NULL, *cwd = NULL;
Expand Down Expand Up @@ -984,7 +998,8 @@ int uv_spawn(uv_loop_t* loop,
}

/* Get PATH environment variable. */
{
path = find_path(env);
if (path == NULL) {
DWORD path_len, r;

path_len = GetEnvironmentVariableW(L"PATH", NULL, 0);
Expand All @@ -993,11 +1008,12 @@ int uv_spawn(uv_loop_t* loop,
goto done;
}

path = (WCHAR*) malloc(path_len * sizeof(WCHAR));
if (path == NULL) {
alloc_path = (WCHAR*) malloc(path_len * sizeof(WCHAR));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't introduce this, but can you remove the unnecessary cast? :-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for leaving the cast in :)

if (alloc_path == NULL) {
err = ERROR_OUTOFMEMORY;
goto done;
}
path = alloc_path;

r = GetEnvironmentVariableW(L"PATH", path, path_len);
if (r == 0 || r >= path_len) {
Expand Down Expand Up @@ -1131,7 +1147,7 @@ int uv_spawn(uv_loop_t* loop,
free(arguments);
free(cwd);
free(env);
free(path);
free(alloc_path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. LGTM then.


if (process->child_stdio_buffer != NULL) {
/* Clean up child stdio handles. */
Expand Down
2 changes: 2 additions & 0 deletions test/test-list.h
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ TEST_DECLARE (spawn_stdout_to_file)
TEST_DECLARE (spawn_stdout_and_stderr_to_file)
TEST_DECLARE (spawn_auto_unref)
TEST_DECLARE (spawn_closed_process_io)
TEST_DECLARE (spawn_reads_child_path)
TEST_DECLARE (fs_poll)
TEST_DECLARE (fs_poll_getpath)
TEST_DECLARE (kill)
Expand Down Expand Up @@ -520,6 +521,7 @@ TASK_LIST_START
TEST_ENTRY (spawn_stdout_and_stderr_to_file)
TEST_ENTRY (spawn_auto_unref)
TEST_ENTRY (spawn_closed_process_io)
TEST_ENTRY (spawn_reads_child_path)
TEST_ENTRY (fs_poll)
TEST_ENTRY (fs_poll_getpath)
TEST_ENTRY (kill)
Expand Down
35 changes: 35 additions & 0 deletions test/test-spawn.c
Original file line number Diff line number Diff line change
Expand Up @@ -1291,3 +1291,38 @@ TEST_IMPL(closed_fd_events) {
return 0;
}
#endif /* !_WIN32 */

TEST_IMPL(spawn_reads_child_path) {
int r;
int len;
char path[1024];
char *env[2] = {path, NULL};

/* Set up the process, but make sure that the file to run is relative and */
/* requires a lookup into PATH */
init_process_options("spawn_helper1", exit_cb);
options.file = "run-tests";
args[0] = "run-tests";

/* Set up the PATH env variable */
for (len = strlen(exepath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't len initialized twice here?

exepath[len - 1] != '/' && exepath[len - 1] != '\\';
len--);
exepath[len] = 0;
strcpy(path, "PATH=");
strcpy(path + 5, exepath);

options.env = env;

r = uv_spawn(uv_default_loop(), &process, &options);
ASSERT(r == 0);

r = uv_run(uv_default_loop(), UV_RUN_DEFAULT);
ASSERT(r == 0);

ASSERT(exit_cb_called == 1);
ASSERT(close_cb_called == 1);

MAKE_VALGRIND_HAPPY();
return 0;
}