- With this patch, the empty path (empty element in PATH or PATH is absent) is treated as the current directory as Linux does. Addresses: https://cygwin.com/pipermail/cygwin/2022-June/251730.html --- winsup/cygwin/release/3.3.6 | 4 ++++ winsup/cygwin/spawn.cc | 13 +++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/winsup/cygwin/release/3.3.6 b/winsup/cygwin/release/3.3.6 index f1a4b7812..ad4069568 100644 --- a/winsup/cygwin/release/3.3.6 +++ b/winsup/cygwin/release/3.3.6 @@ -22,3 +22,7 @@ Bug Fixes if events are inquired in multiple pollfd entries on the same fd at the same time. Addresses: https://cygwin.com/pipermail/cygwin/2022-June/251732.html + +- Treat an empty path (empty element in PATH or PATH is absent) as + the current directory as Linux does. + Addresses: https://cygwin.com/pipermail/cygwin/2022-June/251730.html diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc index e0f1247e1..292cd5a42 100644 --- a/winsup/cygwin/spawn.cc +++ b/winsup/cygwin/spawn.cc @@ -95,6 +95,7 @@ find_exec (const char *name, path_conv& buf, const char *search, char *tmp = tp.c_get (); bool has_slash = !!strpbrk (name, "/\\"); int err = 0; + bool eopath = false; debug_printf ("find_exec (%s)", name); @@ -118,8 +119,10 @@ find_exec (const char *name, path_conv& buf, const char *search, the name of an environment variable. */ if (strchr (search, '/')) *stpncpy (tmp, search, NT_MAX_PATH - 1) = '\0'; - else if (has_slash || isdrive (name) || !(path = getenv (search)) || !*path) + else if (has_slash || isdrive (name)) goto errout; + else if (!(path = getenv (search)) || !*path) + strcpy (tmp, "."); /* Search the current directory when PATH is absent */ else *stpncpy (tmp, path, NT_MAX_PATH - 1) = '\0'; @@ -130,11 +133,17 @@ find_exec (const char *name, path_conv& buf, const char *search, do { char *eotmp = strccpy (tmp_path, &path, ':'); + if (*path) + path++; + else + eopath = true; /* An empty path or '.' means the current directory, but we've already tried that. */ if ((opt & FE_CWD) && (tmp_path[0] == '\0' || (tmp_path[0] == '.' && tmp_path[1] == '\0'))) continue; + else if (tmp_path[0] == '\0') /* An empty path means the current dir. */ + eotmp = stpcpy (tmp_path, "."); *eotmp++ = '/'; stpcpy (eotmp, name); @@ -155,7 +164,7 @@ find_exec (const char *name, path_conv& buf, const char *search, } } - while (*path && *++path); + while (!eopath); errout: /* Couldn't find anything in the given path. -- 2.36.1
On 6/27/2022 8:44 AM, Takashi Yano wrote: > - With this patch, the empty path (empty element in PATH or PATH is > absent) is treated as the current directory as Linux does. > Addresses: https://cygwin.com/pipermail/cygwin/2022-June/251730.html It might be a good idea to include a comment in the code and the commit message that this feature is being added for Linux compatibility but that it is deprecated. According to https://man7.org/linux/man-pages/man7/environ.7.html, As a legacy feature, a zero-length prefix (specified as two adjacent colons, or an initial or terminating colon) is interpreted to mean the current working directory. However, use of this feature is deprecated, and POSIX notes that a conforming application shall use an explicit pathname (e.g., .) to specify the current working directory. Alternatively, maybe this is a case where we should prefer POSIX compliance to Linux compatibility. Corinna, WDYT? Ken
> However, use of this feature is deprecated, and POSIX
> notes that a conforming application shall use an explicit
> pathname (e.g., .) to specify the current working
> directory.
Since "SHALL" does not mean "MUST", I think this patch is correct.
Anton Lavrentiev
Contractor NIH/NLM/NCBI
On 2022-06-30 12:35, Lavrentiev, Anton (NIH/NLM/NCBI) [C] via Cygwin-patches wrote: >> However, use of this feature is deprecated, and POSIX notes that a >> conforming application shall use an explicit pathname (e.g., .) to >> specify the current working directory. > Since "SHALL" does not mean "MUST", I think this patch is correct. It appears you may be confusing POSIX's (1.5 Terminology) *shall* (mandatory) and *should* (recommended): "*SHALL* For an implementation that conforms to POSIX.1-2017, describes a feature or behavior that is mandatory. An application can rely on the existence of the feature or behavior. For an application or user, describes a behavior that is mandatory. *SHOULD* For an implementation that conforms to POSIX.1-2017, describes a feature or behavior that is recommended but not mandatory. An application should not rely on the existence of the feature or behavior. An application that relies on such a feature or behavior cannot be assured to be portable across conforming implementations. For an application, describes a feature or behavior that is recommended programming practice for optimum portability." -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised. [Data in binary units and prefixes, physical quantities in SI.]
On Thu, 30 Jun 2022 18:35:04 +0000 "Lavrentiev, Anton \(NIH/NLM/NCBI\) \[C\] wrote: > > However, use of this feature is deprecated, and POSIX > > notes that a conforming application shall use an explicit > > pathname (e.g., .) to specify the current working > > directory. > > Since "SHALL" does not mean "MUST", I think this patch is correct. In the POSIX standard, "SHALL" is used almost interchangeably with "MUST" as other standard documents do. https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap01.html#tag_21_01_09 -- Takashi Yano <takashi.yano@nifty.ne.jp>
On 6/30/2022 11:45 AM, Ken Brown wrote:
> On 6/27/2022 8:44 AM, Takashi Yano wrote:
>> - With this patch, the empty path (empty element in PATH or PATH is
>> absent) is treated as the current directory as Linux does.
>> Addresses: https://cygwin.com/pipermail/cygwin/2022-June/251730.html
>
> It might be a good idea to include a comment in the code and the commit message
> that this feature is being added for Linux compatibility but that it is
> deprecated. According to https://man7.org/linux/man-pages/man7/environ.7.html,
>
> As a legacy feature, a zero-length prefix (specified as
> two adjacent colons, or an initial or terminating colon)
> is interpreted to mean the current working directory.
> However, use of this feature is deprecated, and POSIX
> notes that a conforming application shall use an explicit
> pathname (e.g., .) to specify the current working
> directory.
>
> Alternatively, maybe this is a case where we should prefer POSIX compliance to
> Linux compatibility. Corinna, WDYT?
I withdraw my suggestion. There's already a comment in the code saying, "An
empty path or '.' means the current directory", so it's clear that the intention
was to support that feature, and the code was simply buggy.
I've now read through the patch, and it looks good to me. This was pretty
tricky to get right.
Ken
On Thu, 30 Jun 2022 21:16:35 -0400
Ken Brown wrote:
> On 6/30/2022 11:45 AM, Ken Brown wrote:
> > On 6/27/2022 8:44 AM, Takashi Yano wrote:
> >> - With this patch, the empty path (empty element in PATH or PATH is
> >> absent) is treated as the current directory as Linux does.
> >> Addresses: https://cygwin.com/pipermail/cygwin/2022-June/251730.html
> >
> > It might be a good idea to include a comment in the code and the commit message
> > that this feature is being added for Linux compatibility but that it is
> > deprecated. According to https://man7.org/linux/man-pages/man7/environ.7.html,
> >
> > As a legacy feature, a zero-length prefix (specified as
> > two adjacent colons, or an initial or terminating colon)
> > is interpreted to mean the current working directory.
> > However, use of this feature is deprecated, and POSIX
> > notes that a conforming application shall use an explicit
> > pathname (e.g., .) to specify the current working
> > directory.
> >
> > Alternatively, maybe this is a case where we should prefer POSIX compliance to
> > Linux compatibility. Corinna, WDYT?
>
> I withdraw my suggestion. There's already a comment in the code saying, "An
> empty path or '.' means the current directory", so it's clear that the intention
> was to support that feature, and the code was simply buggy.
>
> I've now read through the patch, and it looks good to me. This was pretty
> tricky to get right.
We still need to discuss whether it is better to align Linux
behavior or just keeping POSIX compliance, don't we?
--
Takashi Yano <takashi.yano@nifty.ne.jp>
On 7/1/2022 7:31 PM, Takashi Yano wrote:
> On Thu, 30 Jun 2022 21:16:35 -0400
> Ken Brown wrote:
>> On 6/30/2022 11:45 AM, Ken Brown wrote:
>>> On 6/27/2022 8:44 AM, Takashi Yano wrote:
>>>> - With this patch, the empty path (empty element in PATH or PATH is
>>>> absent) is treated as the current directory as Linux does.
>>>> Addresses: https://cygwin.com/pipermail/cygwin/2022-June/251730.html
>>>
>>> It might be a good idea to include a comment in the code and the commit message
>>> that this feature is being added for Linux compatibility but that it is
>>> deprecated. According to https://man7.org/linux/man-pages/man7/environ.7.html,
>>>
>>> As a legacy feature, a zero-length prefix (specified as
>>> two adjacent colons, or an initial or terminating colon)
>>> is interpreted to mean the current working directory.
>>> However, use of this feature is deprecated, and POSIX
>>> notes that a conforming application shall use an explicit
>>> pathname (e.g., .) to specify the current working
>>> directory.
>>>
>>> Alternatively, maybe this is a case where we should prefer POSIX compliance to
>>> Linux compatibility. Corinna, WDYT?
>>
>> I withdraw my suggestion. There's already a comment in the code saying, "An
>> empty path or '.' means the current directory", so it's clear that the intention
>> was to support that feature, and the code was simply buggy.
>>
>> I've now read through the patch, and it looks good to me. This was pretty
>> tricky to get right.
>
> We still need to discuss whether it is better to align Linux
> behavior or just keeping POSIX compliance, don't we?
I interpreted the existing comment as meaning that a decision was already made
at some point to align with Linux. But it can't hurt to wait for Corinna to
weigh in.
Ken
On Jul 1 21:32, Ken Brown wrote:
> On 7/1/2022 7:31 PM, Takashi Yano wrote:
> > On Thu, 30 Jun 2022 21:16:35 -0400
> > Ken Brown wrote:
> > > On 6/30/2022 11:45 AM, Ken Brown wrote:
> > > > On 6/27/2022 8:44 AM, Takashi Yano wrote:
> > > > > - With this patch, the empty path (empty element in PATH or PATH is
> > > > > absent) is treated as the current directory as Linux does.
> > > > > Addresses: https://cygwin.com/pipermail/cygwin/2022-June/251730.html
> > > >
> > > > It might be a good idea to include a comment in the code and the commit message
> > > > that this feature is being added for Linux compatibility but that it is
> > > > deprecated. According to https://man7.org/linux/man-pages/man7/environ.7.html,
> > > >
> > > > As a legacy feature, a zero-length prefix (specified as
> > > > two adjacent colons, or an initial or terminating colon)
> > > > is interpreted to mean the current working directory.
> > > > However, use of this feature is deprecated, and POSIX
> > > > notes that a conforming application shall use an explicit
> > > > pathname (e.g., .) to specify the current working
> > > > directory.
> > > >
> > > > Alternatively, maybe this is a case where we should prefer POSIX compliance to
> > > > Linux compatibility. Corinna, WDYT?
> > >
> > > I withdraw my suggestion. There's already a comment in the code saying, "An
> > > empty path or '.' means the current directory", so it's clear that the intention
> > > was to support that feature, and the code was simply buggy.
> > >
> > > I've now read through the patch, and it looks good to me. This was pretty
> > > tricky to get right.
> >
> > We still need to discuss whether it is better to align Linux
> > behavior or just keeping POSIX compliance, don't we?
>
> I interpreted the existing comment as meaning that a decision was already
> made at some point to align with Linux. But it can't hurt to wait for
> Corinna to weigh in.
Personally I don't like this old crufty feature and I would rather keep
this POSIX compatible, but in fact it was meant to work as on Linux, so,
please go ahead, Takashi.
However, maybe this should go into the master branch only? WDYT?
Corinna
On 7/4/2022 4:37 AM, Corinna Vinschen wrote:
> On Jul 1 21:32, Ken Brown wrote:
>> I interpreted the existing comment as meaning that a decision was already
>> made at some point to align with Linux. But it can't hurt to wait for
>> Corinna to weigh in.
>
> Personally I don't like this old crufty feature and I would rather keep
> this POSIX compatible, but in fact it was meant to work as on Linux, so,
> please go ahead, Takashi.
>
> However, maybe this should go into the master branch only? WDYT?
I think so. The bug has been around for a long time, and the code is tricky.
So we probably shouldn't take a chance on de-stabilizing the 3.3 branch.
Ken
On Mon, 4 Jul 2022 19:05:19 -0400
Ken Brown wrote:
> On 7/4/2022 4:37 AM, Corinna Vinschen wrote:
> > On Jul 1 21:32, Ken Brown wrote:
> >> I interpreted the existing comment as meaning that a decision was already
> >> made at some point to align with Linux. But it can't hurt to wait for
> >> Corinna to weigh in.
> >
> > Personally I don't like this old crufty feature and I would rather keep
> > this POSIX compatible, but in fact it was meant to work as on Linux, so,
> > please go ahead, Takashi.
> >
> > However, maybe this should go into the master branch only? WDYT?
>
> I think so. The bug has been around for a long time, and the code is tricky.
> So we probably shouldn't take a chance on de-stabilizing the 3.3 branch.
Thanks! I have pushed the patch to master branch.
--
Takashi Yano <takashi.yano@nifty.ne.jp>