From: René Scharfe <l.s.r@web.de>
Date: Sat, 20 May 2017 19:35:33 GMT
Subject: Re: [PATCH] mingw: simplify PATH handling
Message-ID: <8fcdecde-2039-c674-3fee-b8e766fbcde7@web.de>
In-Reply-To: <5d4f08c0-07e2-b59c-a132-d165a21323df@kdbg.org>

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é