[PATCH] mingw: simplify PATH handling

[PATCH] mingw: simplify PATH handling

[René Scharfe]

On Windows the environment variable PATH contains a semicolon-separated
list of directories to search for, in order, when looking for the
location of a binary to run.  get_path_split() parses it and returns an
array of string copies, which is iterated by path_lookup(), which in
turn passes each entry to lookup_prog().

Change lookup_prog() to take the directory name as a length-limited
string instead of as a NUL-terminated one and parse PATH directly in
path_lookup().  This avoids memory allocations, simplifying the code.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 compat/mingw.c | 96 ++++++++++++++--------------------------------------------
 1 file changed, 22 insertions(+), 74 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 5113071bc7..7bc61d4066 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1154,67 +1154,15 @@ static const char *parse_interpreter(const char *cmd)
 }
 
 /*
- * Splits the PATH into parts.
- */
-static char **get_path_split(void)
-{
-	char *p, **path, *envpath = mingw_getenv("PATH");
-	int i, n = 0;
-
-	if (!envpath || !*envpath)
-		return NULL;
-
-	envpath = xstrdup(envpath);
-	p = envpath;
-	while (p) {
-		char *dir = p;
-		p = strchr(p, ';');
-		if (p) *p++ = '\0';
-		if (*dir) {	/* not earlier, catches series of ; */
-			++n;
-		}
-	}
-	if (!n) {
-		free(envpath);
-		return NULL;
-	}
-
-	ALLOC_ARRAY(path, n + 1);
-	p = envpath;
-	i = 0;
-	do {
-		if (*p)
-			path[i++] = xstrdup(p);
-		p = p+strlen(p)+1;
-	} while (i < n);
-	path[i] = NULL;
-
-	free(envpath);
-
-	return path;
-}
-
-static void free_path_split(char **path)
-{
-	char **p = path;
-
-	if (!path)
-		return;
-
-	while (*p)
-		free(*p++);
-	free(path);
-}
-
-/*
  * exe_only means that we only want to detect .exe files, but not scripts
  * (which do not have an extension)
  */
-static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_only)
+static char *lookup_prog(const char *dir, int dirlen, const char *cmd,
+			 int isexe, int exe_only)
 {
 	char path[MAX_PATH];
 	wchar_t wpath[MAX_PATH];
-	snprintf(path, sizeof(path), "%s\\%s.exe", dir, cmd);
+	snprintf(path, sizeof(path), "%.*s\\%s.exe", dirlen, dir, cmd);
 
 	if (xutftowcs_path(wpath, path) < 0)
 		return NULL;
@@ -1235,17 +1183,27 @@ static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_on
  * Determines the absolute path of cmd using the split path in path.
  * If cmd contains a slash or backslash, no lookup is performed.
  */
-static char *path_lookup(const char *cmd, char **path, int exe_only)
+static char *path_lookup(const char *cmd, int exe_only)
 {
+	const char *path;
 	char *prog = NULL;
 	int len = strlen(cmd);
 	int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe");
 
 	if (strchr(cmd, '/') || strchr(cmd, '\\'))
-		prog = xstrdup(cmd);
+		return xstrdup(cmd);
 
-	while (!prog && *path)
-		prog = lookup_prog(*path++, cmd, isexe, exe_only);
+	path = mingw_getenv("PATH");
+	if (!path)
+		return NULL;
+
+	for (; !prog && *path; path++) {
+		const char *sep = strchrnul(path, ';');
+		if (sep == path)
+			continue;
+		prog = lookup_prog(path, sep - path, cmd, isexe, exe_only);
+		path = sep;
+	}
 
 	return prog;
 }
@@ -1569,13 +1527,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
 	}
 
 	if (getenv("GIT_STRACE_COMMANDS")) {
-		char **path = get_path_split();
-		char *p = path_lookup("strace.exe", path, 1);
+		char *p = path_lookup("strace.exe", 1);
 		if (!p) {
-			free_path_split(path);
 			return error("strace not found!");
 		}
-		free_path_split(path);
 		if (xutftowcs_path(wcmd, p) < 0) {
 			free(p);
 			return -1;
@@ -1634,8 +1589,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,
 		     int fhin, int fhout, int fherr)
 {
 	pid_t pid;
-	char **path = get_path_split();
-	char *prog = path_lookup(cmd, path, 0);
+	char *prog = path_lookup(cmd, 0);
 
 	if (!prog) {
 		errno = ENOENT;
@@ -1646,7 +1600,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,
 
 		if (interpr) {
 			const char *argv0 = argv[0];
-			char *iprog = path_lookup(interpr, path, 1);
+			char *iprog = path_lookup(interpr, 1);
 			argv[0] = prog;
 			if (!iprog) {
 				errno = ENOENT;
@@ -1664,21 +1618,18 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,
 					       fhin, fhout, fherr);
 		free(prog);
 	}
-	free_path_split(path);
 	return pid;
 }
 
 static int try_shell_exec(const char *cmd, char *const *argv)
 {
 	const char *interpr = parse_interpreter(cmd);
-	char **path;
 	char *prog;
 	int pid = 0;
 
 	if (!interpr)
 		return 0;
-	path = get_path_split();
-	prog = path_lookup(interpr, path, 1);
+	prog = path_lookup(interpr, 1);
 	if (prog) {
 		int argc = 0;
 #ifndef _MSC_VER
@@ -1700,7 +1651,6 @@ static int try_shell_exec(const char *cmd, char *const *argv)
 		free(prog);
 		free(argv2);
 	}
-	free_path_split(path);
 	return pid;
 }
 
@@ -1722,8 +1672,7 @@ int mingw_execv(const char *cmd, char *const *argv)
 
 int mingw_execvp(const char *cmd, char *const *argv)
 {
-	char **path = get_path_split();
-	char *prog = path_lookup(cmd, path, 0);
+	char *prog = path_lookup(cmd, 0);
 
 	if (prog) {
 		mingw_execv(prog, argv);
@@ -1731,7 +1680,6 @@ int mingw_execvp(const char *cmd, char *const *argv)
 	} else
 		errno = ENOENT;
 
-	free_path_split(path);
 	return -1;
 }
 
-- 
2.12.1.windows.1

Re: [PATCH] mingw: simplify PATH handling

[Johannes Sixt]

Am 20.05.2017 um 17:29 schrieb René Scharfe:
> -static char *path_lookup(const char *cmd, char **path, int exe_only)
> +static char *path_lookup(const char *cmd, int exe_only)
>   {
> +	const char *path;
>   	char *prog = NULL;
>   	int len = strlen(cmd);
>   	int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe");
>   
>   	if (strchr(cmd, '/') || strchr(cmd, '\\'))
> -		prog = xstrdup(cmd);
> +		return xstrdup(cmd);
>   
> -	while (!prog && *path)
> -		prog = lookup_prog(*path++, cmd, isexe, exe_only);
> +	path = mingw_getenv("PATH");
> +	if (!path)
> +		return NULL;
> +
> +	for (; !prog && *path; path++) {
> +		const char *sep = strchrnul(path, ';');
> +		if (sep == path)
> +			continue;
> +		prog = lookup_prog(path, sep - path, cmd, isexe, exe_only);
> +		path = sep;
> +	}

The loop termination does not work here. When the final PATH component 
is investigated, sep points to the NUL. This pointer is assigned to 
path, which is incremented and now points one past NUL. Then the loop 
condition (*path) accesses the char behind NUL.

>   
>   	return prog;
>   }
> @@ -1569,13 +1527,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
>   	}
>   
>   	if (getenv("GIT_STRACE_COMMANDS")) {
> -		char **path = get_path_split();
> -		char *p = path_lookup("strace.exe", path, 1);
> +		char *p = path_lookup("strace.exe", 1);
>   		if (!p) {
> -			free_path_split(path);
>   			return error("strace not found!");
>   		}
> -		free_path_split(path);
>   		if (xutftowcs_path(wcmd, p) < 0) {
>   			free(p);
>   			return -1;

Upstream does not have this hunk.

Otherwise, good catch!

-- Hannes

Re: [PATCH] mingw: simplify PATH handling

[René Scharfe]

Am 20.05.2017 um 19:00 schrieb Johannes Sixt:
> Am 20.05.2017 um 17:29 schrieb René Scharfe:
>> -static char *path_lookup(const char *cmd, char **path, int exe_only)
>> +static char *path_lookup(const char *cmd, int exe_only)
>>   {
>> +    const char *path;
>>       char *prog = NULL;
>>       int len = strlen(cmd);
>>       int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe");
>>       if (strchr(cmd, '/') || strchr(cmd, '\\'))
>> -        prog = xstrdup(cmd);
>> +        return xstrdup(cmd);
>> -    while (!prog && *path)
>> -        prog = lookup_prog(*path++, cmd, isexe, exe_only);
>> +    path = mingw_getenv("PATH");
>> +    if (!path)
>> +        return NULL;
>> +
>> +    for (; !prog && *path; path++) {
>> +        const char *sep = strchrnul(path, ';');
>> +        if (sep == path)
>> +            continue;
>> +        prog = lookup_prog(path, sep - path, cmd, isexe, exe_only);
>> +        path = sep;
>> +    }
> 
> The loop termination does not work here. When the final PATH component 
> is investigated, sep points to the NUL. This pointer is assigned to 
> path, which is incremented and now points one past NUL. Then the loop 
> condition (*path) accesses the char behind NUL.

Ugh, yes.  Thanks for catching!

Cause: Last minute/hour edit, had used strspn() before.

>>       return prog;
>>   }
>> @@ -1569,13 +1527,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, 
>> const char **argv, char **deltaen
>>       }
>>       if (getenv("GIT_STRACE_COMMANDS")) {
>> -        char **path = get_path_split();
>> -        char *p = path_lookup("strace.exe", path, 1);
>> +        char *p = path_lookup("strace.exe", 1);
>>           if (!p) {
>> -            free_path_split(path);
>>               return error("strace not found!");
>>           }
>> -        free_path_split(path);
>>           if (xutftowcs_path(wcmd, p) < 0) {
>>               free(p);
>>               return -1;
> 
> Upstream does not have this hunk.


Right, it's in next, but not yet in master.  And there are other
differences, so it's a bad time to do that cleanup. :(

René

[PATCH v2] mingw: simplify PATH handling

[René Scharfe]

On Windows the environment variable PATH contains a semicolon-separated
list of directories to search for, in order, when looking for the
location of a binary to run.  get_path_split() parses it and returns an
array of string copies, which is iterated by path_lookup(), which in
turn passes each entry to lookup_prog().

Change lookup_prog() to take the directory name as a length-limited
string instead of as a NUL-terminated one and parse PATH directly in
path_lookup().  This avoids memory allocations, simplifying the code.

Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Rebased against Junio's master, fixed string overrun.  Can hold and
resubmit in a few months if it gets in the way right now.

 compat/mingw.c | 91 +++++++++++++++-------------------------------------------
 1 file changed, 23 insertions(+), 68 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5978..c6134f7c81 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -941,64 +941,14 @@ static const char *parse_interpreter(const char *cmd)
 }
 
 /*
- * Splits the PATH into parts.
- */
-static char **get_path_split(void)
-{
-	char *p, **path, *envpath = mingw_getenv("PATH");
-	int i, n = 0;
-
-	if (!envpath || !*envpath)
-		return NULL;
-
-	envpath = xstrdup(envpath);
-	p = envpath;
-	while (p) {
-		char *dir = p;
-		p = strchr(p, ';');
-		if (p) *p++ = '\0';
-		if (*dir) {	/* not earlier, catches series of ; */
-			++n;
-		}
-	}
-	if (!n)
-		return NULL;
-
-	ALLOC_ARRAY(path, n + 1);
-	p = envpath;
-	i = 0;
-	do {
-		if (*p)
-			path[i++] = xstrdup(p);
-		p = p+strlen(p)+1;
-	} while (i < n);
-	path[i] = NULL;
-
-	free(envpath);
-
-	return path;
-}
-
-static void free_path_split(char **path)
-{
-	char **p = path;
-
-	if (!path)
-		return;
-
-	while (*p)
-		free(*p++);
-	free(path);
-}
-
-/*
  * exe_only means that we only want to detect .exe files, but not scripts
  * (which do not have an extension)
  */
-static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_only)
+static char *lookup_prog(const char *dir, int dirlen, const char *cmd,
+			 int isexe, int exe_only)
 {
 	char path[MAX_PATH];
-	snprintf(path, sizeof(path), "%s/%s.exe", dir, cmd);
+	snprintf(path, sizeof(path), "%.*s\\%s.exe", dirlen, dir, cmd);
 
 	if (!isexe && access(path, F_OK) == 0)
 		return xstrdup(path);
@@ -1013,17 +963,29 @@ static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_on
  * Determines the absolute path of cmd using the split path in path.
  * If cmd contains a slash or backslash, no lookup is performed.
  */
-static char *path_lookup(const char *cmd, char **path, int exe_only)
+static char *path_lookup(const char *cmd, int exe_only)
 {
+	const char *path;
 	char *prog = NULL;
 	int len = strlen(cmd);
 	int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe");
 
 	if (strchr(cmd, '/') || strchr(cmd, '\\'))
-		prog = xstrdup(cmd);
+		return xstrdup(cmd);
+
+	path = mingw_getenv("PATH");
+	if (!path)
+		return NULL;
 
-	while (!prog && *path)
-		prog = lookup_prog(*path++, cmd, isexe, exe_only);
+	while (!prog) {
+		const char *sep = strchrnul(path, ';');
+		int dirlen = sep - path;
+		if (dirlen)
+			prog = lookup_prog(path, dirlen, cmd, isexe, exe_only);
+		if (!*sep)
+			break;
+		path = sep + 1;
+	}
 
 	return prog;
 }
@@ -1190,8 +1152,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,
 		     int fhin, int fhout, int fherr)
 {
 	pid_t pid;
-	char **path = get_path_split();
-	char *prog = path_lookup(cmd, path, 0);
+	char *prog = path_lookup(cmd, 0);
 
 	if (!prog) {
 		errno = ENOENT;
@@ -1202,7 +1163,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,
 
 		if (interpr) {
 			const char *argv0 = argv[0];
-			char *iprog = path_lookup(interpr, path, 1);
+			char *iprog = path_lookup(interpr, 1);
 			argv[0] = prog;
 			if (!iprog) {
 				errno = ENOENT;
@@ -1220,21 +1181,18 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,
 					       fhin, fhout, fherr);
 		free(prog);
 	}
-	free_path_split(path);
 	return pid;
 }
 
 static int try_shell_exec(const char *cmd, char *const *argv)
 {
 	const char *interpr = parse_interpreter(cmd);
-	char **path;
 	char *prog;
 	int pid = 0;
 
 	if (!interpr)
 		return 0;
-	path = get_path_split();
-	prog = path_lookup(interpr, path, 1);
+	prog = path_lookup(interpr, 1);
 	if (prog) {
 		int argc = 0;
 		const char **argv2;
@@ -1253,7 +1211,6 @@ static int try_shell_exec(const char *cmd, char *const *argv)
 		free(prog);
 		free(argv2);
 	}
-	free_path_split(path);
 	return pid;
 }
 
@@ -1275,8 +1232,7 @@ int mingw_execv(const char *cmd, char *const *argv)
 
 int mingw_execvp(const char *cmd, char *const *argv)
 {
-	char **path = get_path_split();
-	char *prog = path_lookup(cmd, path, 0);
+	char *prog = path_lookup(cmd, 0);
 
 	if (prog) {
 		mingw_execv(prog, argv);
@@ -1284,7 +1240,6 @@ int mingw_execvp(const char *cmd, char *const *argv)
 	} else
 		errno = ENOENT;
 
-	free_path_split(path);
 	return -1;
 }
 
-- 
2.12.1.windows.1

Re: [PATCH v2] mingw: simplify PATH handling

[Johannes Schindelin]

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

Hi René,

On Sat, 20 May 2017, René Scharfe wrote:

> On Windows the environment variable PATH contains a semicolon-separated
> list of directories to search for, in order, when looking for the
> location of a binary to run.  get_path_split() parses it and returns an
> array of string copies, which is iterated by path_lookup(), which in
> turn passes each entry to lookup_prog().
> 
> Change lookup_prog() to take the directory name as a length-limited
> string instead of as a NUL-terminated one and parse PATH directly in
> path_lookup().  This avoids memory allocations, simplifying the code.
> 
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>

ACK.

> Rebased against Junio's master, fixed string overrun.  Can hold and
> resubmit in a few months if it gets in the way right now.

I am in favor of getting this in as soon as possible.

Ciao,
Dscho