{"thread":{"id":"46009","subject":"[PATCH] mingw: simplify PATH handling","startedAt":"2017-05-20T15:29:33Z","lastAt":"2017-05-22T12:58:50Z","messageCount":5,"participants":["René Scharfe","Johannes Sixt","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"320316","messageId":"c5b7292f-6367-e4a9-2ee0-96b93b1b587f@web.de","threadId":"46009","inReplyTo":null,"subject":"[PATCH] mingw: simplify PATH handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-20T15:29:18Z","receivedAt":"2017-05-20T15:29:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On Windows the environment variable PATH contains a semicolon-separated\nlist of directories to search for, in order, when looking for the\nlocation of a binary to run.  get_path_split() parses it and returns an\narray of string copies, which is iterated by path_lookup(), which in\nturn passes each entry to lookup_prog().\n\nChange lookup_prog() to take the directory name as a length-limited\nstring instead of as a NUL-terminated one and parse PATH directly in\npath_lookup().  This avoids memory allocations, simplifying the code.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n compat/mingw.c | 96 ++++++++++++++--------------------------------------------\n 1 file changed, 22 insertions(+), 74 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 5113071bc7..7bc61d4066 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1154,67 +1154,15 @@ static const char *parse_interpreter(const char *cmd)\n }\n \n /*\n- * Splits the PATH into parts.\n- */\n-static char **get_path_split(void)\n-{\n-\tchar *p, **path, *envpath = mingw_getenv(\"PATH\");\n-\tint i, n = 0;\n-\n-\tif (!envpath || !*envpath)\n-\t\treturn NULL;\n-\n-\tenvpath = xstrdup(envpath);\n-\tp = envpath;\n-\twhile (p) {\n-\t\tchar *dir = p;\n-\t\tp = strchr(p, ';');\n-\t\tif (p) *p++ = '\\0';\n-\t\tif (*dir) {\t/* not earlier, catches series of ; */\n-\t\t\t++n;\n-\t\t}\n-\t}\n-\tif (!n) {\n-\t\tfree(envpath);\n-\t\treturn NULL;\n-\t}\n-\n-\tALLOC_ARRAY(path, n + 1);\n-\tp = envpath;\n-\ti = 0;\n-\tdo {\n-\t\tif (*p)\n-\t\t\tpath[i++] = xstrdup(p);\n-\t\tp = p+strlen(p)+1;\n-\t} while (i < n);\n-\tpath[i] = NULL;\n-\n-\tfree(envpath);\n-\n-\treturn path;\n-}\n-\n-static void free_path_split(char **path)\n-{\n-\tchar **p = path;\n-\n-\tif (!path)\n-\t\treturn;\n-\n-\twhile (*p)\n-\t\tfree(*p++);\n-\tfree(path);\n-}\n-\n-/*\n  * exe_only means that we only want to detect .exe files, but not scripts\n  * (which do not have an extension)\n  */\n-static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_only)\n+static char *lookup_prog(const char *dir, int dirlen, const char *cmd,\n+\t\t\t int isexe, int exe_only)\n {\n \tchar path[MAX_PATH];\n \twchar_t wpath[MAX_PATH];\n-\tsnprintf(path, sizeof(path), \"%s\\\\%s.exe\", dir, cmd);\n+\tsnprintf(path, sizeof(path), \"%.*s\\\\%s.exe\", dirlen, dir, cmd);\n \n \tif (xutftowcs_path(wpath, path) < 0)\n \t\treturn NULL;\n@@ -1235,17 +1183,27 @@ static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_on\n  * Determines the absolute path of cmd using the split path in path.\n  * If cmd contains a slash or backslash, no lookup is performed.\n  */\n-static char *path_lookup(const char *cmd, char **path, int exe_only)\n+static char *path_lookup(const char *cmd, int exe_only)\n {\n+\tconst char *path;\n \tchar *prog = NULL;\n \tint len = strlen(cmd);\n \tint isexe = len >= 4 && !strcasecmp(cmd+len-4, \".exe\");\n \n \tif (strchr(cmd, '/') || strchr(cmd, '\\\\'))\n-\t\tprog = xstrdup(cmd);\n+\t\treturn xstrdup(cmd);\n \n-\twhile (!prog && *path)\n-\t\tprog = lookup_prog(*path++, cmd, isexe, exe_only);\n+\tpath = mingw_getenv(\"PATH\");\n+\tif (!path)\n+\t\treturn NULL;\n+\n+\tfor (; !prog && *path; path++) {\n+\t\tconst char *sep = strchrnul(path, ';');\n+\t\tif (sep == path)\n+\t\t\tcontinue;\n+\t\tprog = lookup_prog(path, sep - path, cmd, isexe, exe_only);\n+\t\tpath = sep;\n+\t}\n \n \treturn prog;\n }\n@@ -1569,13 +1527,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n \t}\n \n \tif (getenv(\"GIT_STRACE_COMMANDS\")) {\n-\t\tchar **path = get_path_split();\n-\t\tchar *p = path_lookup(\"strace.exe\", path, 1);\n+\t\tchar *p = path_lookup(\"strace.exe\", 1);\n \t\tif (!p) {\n-\t\t\tfree_path_split(path);\n \t\t\treturn error(\"strace not found!\");\n \t\t}\n-\t\tfree_path_split(path);\n \t\tif (xutftowcs_path(wcmd, p) < 0) {\n \t\t\tfree(p);\n \t\t\treturn -1;\n@@ -1634,8 +1589,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,\n \t\t     int fhin, int fhout, int fherr)\n {\n \tpid_t pid;\n-\tchar **path = get_path_split();\n-\tchar *prog = path_lookup(cmd, path, 0);\n+\tchar *prog = path_lookup(cmd, 0);\n \n \tif (!prog) {\n \t\terrno = ENOENT;\n@@ -1646,7 +1600,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,\n \n \t\tif (interpr) {\n \t\t\tconst char *argv0 = argv[0];\n-\t\t\tchar *iprog = path_lookup(interpr, path, 1);\n+\t\t\tchar *iprog = path_lookup(interpr, 1);\n \t\t\targv[0] = prog;\n \t\t\tif (!iprog) {\n \t\t\t\terrno = ENOENT;\n@@ -1664,21 +1618,18 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,\n \t\t\t\t\t       fhin, fhout, fherr);\n \t\tfree(prog);\n \t}\n-\tfree_path_split(path);\n \treturn pid;\n }\n \n static int try_shell_exec(const char *cmd, char *const *argv)\n {\n \tconst char *interpr = parse_interpreter(cmd);\n-\tchar **path;\n \tchar *prog;\n \tint pid = 0;\n \n \tif (!interpr)\n \t\treturn 0;\n-\tpath = get_path_split();\n-\tprog = path_lookup(interpr, path, 1);\n+\tprog = path_lookup(interpr, 1);\n \tif (prog) {\n \t\tint argc = 0;\n #ifndef _MSC_VER\n@@ -1700,7 +1651,6 @@ static int try_shell_exec(const char *cmd, char *const *argv)\n \t\tfree(prog);\n \t\tfree(argv2);\n \t}\n-\tfree_path_split(path);\n \treturn pid;\n }\n \n@@ -1722,8 +1672,7 @@ int mingw_execv(const char *cmd, char *const *argv)\n \n int mingw_execvp(const char *cmd, char *const *argv)\n {\n-\tchar **path = get_path_split();\n-\tchar *prog = path_lookup(cmd, path, 0);\n+\tchar *prog = path_lookup(cmd, 0);\n \n \tif (prog) {\n \t\tmingw_execv(prog, argv);\n@@ -1731,7 +1680,6 @@ int mingw_execvp(const char *cmd, char *const *argv)\n \t} else\n \t\terrno = ENOENT;\n \n-\tfree_path_split(path);\n \treturn -1;\n }\n \n-- \n2.12.1.windows.1\n\n"},{"id":"320318","messageId":"5d4f08c0-07e2-b59c-a132-d165a21323df@kdbg.org","threadId":"46009","inReplyTo":"c5b7292f-6367-e4a9-2ee0-96b93b1b587f@web.de","subject":"Re: [PATCH] mingw: simplify PATH handling","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-20T17:00:18Z","receivedAt":"2017-05-20T17:00:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.05.2017 um 17:29 schrieb René Scharfe:\n> -static char *path_lookup(const char *cmd, char **path, int exe_only)\n> +static char *path_lookup(const char *cmd, int exe_only)\n>   {\n> +\tconst char *path;\n>   \tchar *prog = NULL;\n>   \tint len = strlen(cmd);\n>   \tint isexe = len >= 4 && !strcasecmp(cmd+len-4, \".exe\");\n>   \n>   \tif (strchr(cmd, '/') || strchr(cmd, '\\\\'))\n> -\t\tprog = xstrdup(cmd);\n> +\t\treturn xstrdup(cmd);\n>   \n> -\twhile (!prog && *path)\n> -\t\tprog = lookup_prog(*path++, cmd, isexe, exe_only);\n> +\tpath = mingw_getenv(\"PATH\");\n> +\tif (!path)\n> +\t\treturn NULL;\n> +\n> +\tfor (; !prog && *path; path++) {\n> +\t\tconst char *sep = strchrnul(path, ';');\n> +\t\tif (sep == path)\n> +\t\t\tcontinue;\n> +\t\tprog = lookup_prog(path, sep - path, cmd, isexe, exe_only);\n> +\t\tpath = sep;\n> +\t}\n\nThe loop termination does not work here. When the final PATH component \nis investigated, sep points to the NUL. This pointer is assigned to \npath, which is incremented and now points one past NUL. Then the loop \ncondition (*path) accesses the char behind NUL.\n\n>   \n>   \treturn prog;\n>   }\n> @@ -1569,13 +1527,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n>   \t}\n>   \n>   \tif (getenv(\"GIT_STRACE_COMMANDS\")) {\n> -\t\tchar **path = get_path_split();\n> -\t\tchar *p = path_lookup(\"strace.exe\", path, 1);\n> +\t\tchar *p = path_lookup(\"strace.exe\", 1);\n>   \t\tif (!p) {\n> -\t\t\tfree_path_split(path);\n>   \t\t\treturn error(\"strace not found!\");\n>   \t\t}\n> -\t\tfree_path_split(path);\n>   \t\tif (xutftowcs_path(wcmd, p) < 0) {\n>   \t\t\tfree(p);\n>   \t\t\treturn -1;\n\nUpstream does not have this hunk.\n\nOtherwise, good catch!\n\n-- Hannes\n"},{"id":"320320","messageId":"8fcdecde-2039-c674-3fee-b8e766fbcde7@web.de","threadId":"46009","inReplyTo":"5d4f08c0-07e2-b59c-a132-d165a21323df@kdbg.org","subject":"Re: [PATCH] mingw: simplify PATH handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-20T19:35:33Z","receivedAt":"2017-05-20T19:35:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.05.2017 um 19:00 schrieb Johannes Sixt:\n> Am 20.05.2017 um 17:29 schrieb René Scharfe:\n>> -static char *path_lookup(const char *cmd, char **path, int exe_only)\n>> +static char *path_lookup(const char *cmd, int exe_only)\n>>   {\n>> +    const char *path;\n>>       char *prog = NULL;\n>>       int len = strlen(cmd);\n>>       int isexe = len >= 4 && !strcasecmp(cmd+len-4, \".exe\");\n>>       if (strchr(cmd, '/') || strchr(cmd, '\\\\'))\n>> -        prog = xstrdup(cmd);\n>> +        return xstrdup(cmd);\n>> -    while (!prog && *path)\n>> -        prog = lookup_prog(*path++, cmd, isexe, exe_only);\n>> +    path = mingw_getenv(\"PATH\");\n>> +    if (!path)\n>> +        return NULL;\n>> +\n>> +    for (; !prog && *path; path++) {\n>> +        const char *sep = strchrnul(path, ';');\n>> +        if (sep == path)\n>> +            continue;\n>> +        prog = lookup_prog(path, sep - path, cmd, isexe, exe_only);\n>> +        path = sep;\n>> +    }\n> \n> The loop termination does not work here. When the final PATH component \n> is investigated, sep points to the NUL. This pointer is assigned to \n> path, which is incremented and now points one past NUL. Then the loop \n> condition (*path) accesses the char behind NUL.\n\nUgh, yes.  Thanks for catching!\n\nCause: Last minute/hour edit, had used strspn() before.\n\n>>       return prog;\n>>   }\n>> @@ -1569,13 +1527,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, \n>> const char **argv, char **deltaen\n>>       }\n>>       if (getenv(\"GIT_STRACE_COMMANDS\")) {\n>> -        char **path = get_path_split();\n>> -        char *p = path_lookup(\"strace.exe\", path, 1);\n>> +        char *p = path_lookup(\"strace.exe\", 1);\n>>           if (!p) {\n>> -            free_path_split(path);\n>>               return error(\"strace not found!\");\n>>           }\n>> -        free_path_split(path);\n>>           if (xutftowcs_path(wcmd, p) < 0) {\n>>               free(p);\n>>               return -1;\n> \n> Upstream does not have this hunk.\n\n\nRight, it's in next, but not yet in master.  And there are other\ndifferences, so it's a bad time to do that cleanup. :(\n\nRené\n"},{"id":"320321","messageId":"f4119dba-0e25-0a0b-0796-dc4c3d4d1cc5@web.de","threadId":"46009","inReplyTo":"c5b7292f-6367-e4a9-2ee0-96b93b1b587f@web.de","subject":"[PATCH v2] mingw: simplify PATH handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-20T19:35:37Z","receivedAt":"2017-05-20T19:35:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On Windows the environment variable PATH contains a semicolon-separated\nlist of directories to search for, in order, when looking for the\nlocation of a binary to run.  get_path_split() parses it and returns an\narray of string copies, which is iterated by path_lookup(), which in\nturn passes each entry to lookup_prog().\n\nChange lookup_prog() to take the directory name as a length-limited\nstring instead of as a NUL-terminated one and parse PATH directly in\npath_lookup().  This avoids memory allocations, simplifying the code.\n\nHelped-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nRebased against Junio's master, fixed string overrun.  Can hold and\nresubmit in a few months if it gets in the way right now.\n\n compat/mingw.c | 91 +++++++++++++++-------------------------------------------\n 1 file changed, 23 insertions(+), 68 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 3fbfda5978..c6134f7c81 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -941,64 +941,14 @@ static const char *parse_interpreter(const char *cmd)\n }\n \n /*\n- * Splits the PATH into parts.\n- */\n-static char **get_path_split(void)\n-{\n-\tchar *p, **path, *envpath = mingw_getenv(\"PATH\");\n-\tint i, n = 0;\n-\n-\tif (!envpath || !*envpath)\n-\t\treturn NULL;\n-\n-\tenvpath = xstrdup(envpath);\n-\tp = envpath;\n-\twhile (p) {\n-\t\tchar *dir = p;\n-\t\tp = strchr(p, ';');\n-\t\tif (p) *p++ = '\\0';\n-\t\tif (*dir) {\t/* not earlier, catches series of ; */\n-\t\t\t++n;\n-\t\t}\n-\t}\n-\tif (!n)\n-\t\treturn NULL;\n-\n-\tALLOC_ARRAY(path, n + 1);\n-\tp = envpath;\n-\ti = 0;\n-\tdo {\n-\t\tif (*p)\n-\t\t\tpath[i++] = xstrdup(p);\n-\t\tp = p+strlen(p)+1;\n-\t} while (i < n);\n-\tpath[i] = NULL;\n-\n-\tfree(envpath);\n-\n-\treturn path;\n-}\n-\n-static void free_path_split(char **path)\n-{\n-\tchar **p = path;\n-\n-\tif (!path)\n-\t\treturn;\n-\n-\twhile (*p)\n-\t\tfree(*p++);\n-\tfree(path);\n-}\n-\n-/*\n  * exe_only means that we only want to detect .exe files, but not scripts\n  * (which do not have an extension)\n  */\n-static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_only)\n+static char *lookup_prog(const char *dir, int dirlen, const char *cmd,\n+\t\t\t int isexe, int exe_only)\n {\n \tchar path[MAX_PATH];\n-\tsnprintf(path, sizeof(path), \"%s/%s.exe\", dir, cmd);\n+\tsnprintf(path, sizeof(path), \"%.*s\\\\%s.exe\", dirlen, dir, cmd);\n \n \tif (!isexe && access(path, F_OK) == 0)\n \t\treturn xstrdup(path);\n@@ -1013,17 +963,29 @@ static char *lookup_prog(const char *dir, const char *cmd, int isexe, int exe_on\n  * Determines the absolute path of cmd using the split path in path.\n  * If cmd contains a slash or backslash, no lookup is performed.\n  */\n-static char *path_lookup(const char *cmd, char **path, int exe_only)\n+static char *path_lookup(const char *cmd, int exe_only)\n {\n+\tconst char *path;\n \tchar *prog = NULL;\n \tint len = strlen(cmd);\n \tint isexe = len >= 4 && !strcasecmp(cmd+len-4, \".exe\");\n \n \tif (strchr(cmd, '/') || strchr(cmd, '\\\\'))\n-\t\tprog = xstrdup(cmd);\n+\t\treturn xstrdup(cmd);\n+\n+\tpath = mingw_getenv(\"PATH\");\n+\tif (!path)\n+\t\treturn NULL;\n \n-\twhile (!prog && *path)\n-\t\tprog = lookup_prog(*path++, cmd, isexe, exe_only);\n+\twhile (!prog) {\n+\t\tconst char *sep = strchrnul(path, ';');\n+\t\tint dirlen = sep - path;\n+\t\tif (dirlen)\n+\t\t\tprog = lookup_prog(path, dirlen, cmd, isexe, exe_only);\n+\t\tif (!*sep)\n+\t\t\tbreak;\n+\t\tpath = sep + 1;\n+\t}\n \n \treturn prog;\n }\n@@ -1190,8 +1152,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,\n \t\t     int fhin, int fhout, int fherr)\n {\n \tpid_t pid;\n-\tchar **path = get_path_split();\n-\tchar *prog = path_lookup(cmd, path, 0);\n+\tchar *prog = path_lookup(cmd, 0);\n \n \tif (!prog) {\n \t\terrno = ENOENT;\n@@ -1202,7 +1163,7 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,\n \n \t\tif (interpr) {\n \t\t\tconst char *argv0 = argv[0];\n-\t\t\tchar *iprog = path_lookup(interpr, path, 1);\n+\t\t\tchar *iprog = path_lookup(interpr, 1);\n \t\t\targv[0] = prog;\n \t\t\tif (!iprog) {\n \t\t\t\terrno = ENOENT;\n@@ -1220,21 +1181,18 @@ pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **deltaenv,\n \t\t\t\t\t       fhin, fhout, fherr);\n \t\tfree(prog);\n \t}\n-\tfree_path_split(path);\n \treturn pid;\n }\n \n static int try_shell_exec(const char *cmd, char *const *argv)\n {\n \tconst char *interpr = parse_interpreter(cmd);\n-\tchar **path;\n \tchar *prog;\n \tint pid = 0;\n \n \tif (!interpr)\n \t\treturn 0;\n-\tpath = get_path_split();\n-\tprog = path_lookup(interpr, path, 1);\n+\tprog = path_lookup(interpr, 1);\n \tif (prog) {\n \t\tint argc = 0;\n \t\tconst char **argv2;\n@@ -1253,7 +1211,6 @@ static int try_shell_exec(const char *cmd, char *const *argv)\n \t\tfree(prog);\n \t\tfree(argv2);\n \t}\n-\tfree_path_split(path);\n \treturn pid;\n }\n \n@@ -1275,8 +1232,7 @@ int mingw_execv(const char *cmd, char *const *argv)\n \n int mingw_execvp(const char *cmd, char *const *argv)\n {\n-\tchar **path = get_path_split();\n-\tchar *prog = path_lookup(cmd, path, 0);\n+\tchar *prog = path_lookup(cmd, 0);\n \n \tif (prog) {\n \t\tmingw_execv(prog, argv);\n@@ -1284,7 +1240,6 @@ int mingw_execvp(const char *cmd, char *const *argv)\n \t} else\n \t\terrno = ENOENT;\n \n-\tfree_path_split(path);\n \treturn -1;\n }\n \n-- \n2.12.1.windows.1\n"},{"id":"320409","messageId":"alpine.DEB.2.21.1.1705221457520.3610@virtualbox","threadId":"46009","inReplyTo":"f4119dba-0e25-0a0b-0796-dc4c3d4d1cc5@web.de","subject":"Re: [PATCH v2] mingw: simplify PATH handling","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-22T12:58:27Z","receivedAt":"2017-05-22T12:58:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Sat, 20 May 2017, René Scharfe wrote:\n\n> On Windows the environment variable PATH contains a semicolon-separated\n> list of directories to search for, in order, when looking for the\n> location of a binary to run.  get_path_split() parses it and returns an\n> array of string copies, which is iterated by path_lookup(), which in\n> turn passes each entry to lookup_prog().\n> \n> Change lookup_prog() to take the directory name as a length-limited\n> string instead of as a NUL-terminated one and parse PATH directly in\n> path_lookup().  This avoids memory allocations, simplifying the code.\n> \n> Helped-by: Johannes Sixt <j6t@kdbg.org>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n\nACK.\n\n> Rebased against Junio's master, fixed string overrun.  Can hold and\n> resubmit in a few months if it gets in the way right now.\n\nI am in favor of getting this in as soon as possible.\n\nCiao,\nDscho"}]}