public inbox for cygwin-patches@cygwin.com
 help / color / mirror / Atom feed
* [PATCH] Cygwin: spawn: Treat empty path as the current directory.
@ 2022-06-27 12:44 Takashi Yano
  2022-06-30 15:45 ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Takashi Yano @ 2022-06-27 12:44 UTC (permalink / raw)
  To: cygwin-patches

- 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-06-27 12:44 [PATCH] Cygwin: spawn: Treat empty path as the current directory Takashi Yano
@ 2022-06-30 15:45 ` Ken Brown
  2022-06-30 18:35   ` [EXTERNAL] " Lavrentiev, Anton (NIH/NLM/NCBI) [C]
  2022-07-01  1:16   ` Ken Brown
  0 siblings, 2 replies; 11+ messages in thread
From: Ken Brown @ 2022-06-30 15:45 UTC (permalink / raw)
  To: cygwin-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [EXTERNAL] Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-06-30 15:45 ` Ken Brown
@ 2022-06-30 18:35   ` Lavrentiev, Anton (NIH/NLM/NCBI) [C]
  2022-06-30 19:03     ` Brian Inglis
  2022-06-30 19:04     ` Takashi Yano
  2022-07-01  1:16   ` Ken Brown
  1 sibling, 2 replies; 11+ messages in thread
From: Lavrentiev, Anton (NIH/NLM/NCBI) [C] @ 2022-06-30 18:35 UTC (permalink / raw)
  To: Ken Brown, cygwin-patches

>                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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [EXTERNAL] Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-06-30 18:35   ` [EXTERNAL] " Lavrentiev, Anton (NIH/NLM/NCBI) [C]
@ 2022-06-30 19:03     ` Brian Inglis
  2022-06-30 19:04     ` Takashi Yano
  1 sibling, 0 replies; 11+ messages in thread
From: Brian Inglis @ 2022-06-30 19:03 UTC (permalink / raw)
  To: cygwin-patches


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [EXTERNAL] Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-06-30 18:35   ` [EXTERNAL] " Lavrentiev, Anton (NIH/NLM/NCBI) [C]
  2022-06-30 19:03     ` Brian Inglis
@ 2022-06-30 19:04     ` Takashi Yano
  1 sibling, 0 replies; 11+ messages in thread
From: Takashi Yano @ 2022-06-30 19:04 UTC (permalink / raw)
  To: cygwin-patches

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-06-30 15:45 ` Ken Brown
  2022-06-30 18:35   ` [EXTERNAL] " Lavrentiev, Anton (NIH/NLM/NCBI) [C]
@ 2022-07-01  1:16   ` Ken Brown
  2022-07-01 23:31     ` Takashi Yano
  1 sibling, 1 reply; 11+ messages in thread
From: Ken Brown @ 2022-07-01  1:16 UTC (permalink / raw)
  To: cygwin-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-07-01  1:16   ` Ken Brown
@ 2022-07-01 23:31     ` Takashi Yano
  2022-07-02  1:32       ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Takashi Yano @ 2022-07-01 23:31 UTC (permalink / raw)
  To: cygwin-patches

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-07-01 23:31     ` Takashi Yano
@ 2022-07-02  1:32       ` Ken Brown
  2022-07-04  8:37         ` Corinna Vinschen
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2022-07-02  1:32 UTC (permalink / raw)
  To: cygwin-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-07-02  1:32       ` Ken Brown
@ 2022-07-04  8:37         ` Corinna Vinschen
  2022-07-04 23:05           ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Corinna Vinschen @ 2022-07-04  8:37 UTC (permalink / raw)
  To: cygwin-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-07-04  8:37         ` Corinna Vinschen
@ 2022-07-04 23:05           ` Ken Brown
  2022-07-05  4:54             ` Takashi Yano
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2022-07-04 23:05 UTC (permalink / raw)
  To: cygwin-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Cygwin: spawn: Treat empty path as the current directory.
  2022-07-04 23:05           ` Ken Brown
@ 2022-07-05  4:54             ` Takashi Yano
  0 siblings, 0 replies; 11+ messages in thread
From: Takashi Yano @ 2022-07-05  4:54 UTC (permalink / raw)
  To: cygwin-patches

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-07-05  4:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-27 12:44 [PATCH] Cygwin: spawn: Treat empty path as the current directory Takashi Yano
2022-06-30 15:45 ` Ken Brown
2022-06-30 18:35   ` [EXTERNAL] " Lavrentiev, Anton (NIH/NLM/NCBI) [C]
2022-06-30 19:03     ` Brian Inglis
2022-06-30 19:04     ` Takashi Yano
2022-07-01  1:16   ` Ken Brown
2022-07-01 23:31     ` Takashi Yano
2022-07-02  1:32       ` Ken Brown
2022-07-04  8:37         ` Corinna Vinschen
2022-07-04 23:05           ` Ken Brown
2022-07-05  4:54             ` Takashi Yano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).