* [PATCH 0/2] Fix a bad case of absolute path handling
@ 2021-11-10 20:32 corinna-cygwin
2021-11-10 20:32 ` [PATCH 1/2] Cygwin: drop unused isabspath_u and iswabspath macros corinna-cygwin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: corinna-cygwin @ 2021-11-10 20:32 UTC (permalink / raw)
To: cygwin-patches
From: Corinna Vinschen <corinna@vinschen.de>
As I told Takashi in PM, I will try to more often send patches to the
cygwin-patches ML before pushing them, so there's a chance to chime in.
This patch series is supposed to address the `rm -rf' problem reported
in https://cygwin.com/pipermail/cygwin/2021-November/249837.html
It was always frustrating, having to allow DOS drive letter paths for
backward compatibility. This here is another case of ambiguity,
triggered by the `isabspath' macro handling "X:" as absolute path, even
without the trailing slash or backslash.
Check out the 2nd patch for a more detailed description.
While at it, I wonder if we might have a chance to fix these ambiguities
in a better way. For instance, consider this:
$ mkdir -p test/c:
$ cd test
As non-admin:
$ touch c:/foo
touch: cannot touch 'c:/foo': Permission denied
As admin, even worse:
$ touch c:/foo
$ ls /cygdrive/c/foo
foo
As long as we support DOS paths as input, I have a hard time to see how
to fix this, but maybe we can at least minimize the ambiguity somehow.
Corinna
Corinna Vinschen (2):
Cygwin: drop unused isabspath_u and iswabspath macros
Cygwin: introduce isabspath_strict macro
winsup/cygwin/syscalls.cc | 2 +-
winsup/cygwin/winsup.h | 20 ++++++++------------
2 files changed, 9 insertions(+), 13 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] Cygwin: drop unused isabspath_u and iswabspath macros
2021-11-10 20:32 [PATCH 0/2] Fix a bad case of absolute path handling corinna-cygwin
@ 2021-11-10 20:32 ` corinna-cygwin
2021-11-10 20:32 ` [PATCH 2/2] Cygwin: introduce isabspath_strict macro corinna-cygwin
2021-11-10 22:22 ` [PATCH 0/2] Fix a bad case of absolute path handling Ken Brown
2 siblings, 0 replies; 7+ messages in thread
From: corinna-cygwin @ 2021-11-10 20:32 UTC (permalink / raw)
To: cygwin-patches
From: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
winsup/cygwin/winsup.h | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/winsup/cygwin/winsup.h b/winsup/cygwin/winsup.h
index abdef35261ca..f6fea6313d56 100644
--- a/winsup/cygwin/winsup.h
+++ b/winsup/cygwin/winsup.h
@@ -139,18 +139,6 @@ extern int cygserver_running;
#undef issep
#define issep(ch) (strchr (" \t\n\r", (ch)) != NULL)
-/* Every path beginning with / or \, as well as every path being X:
- or starting with X:/ or X:\ */
-#define isabspath_u(p) \
- ((p)->Length && \
- (iswdirsep ((p)->Buffer[0]) || \
- ((p)->Length > sizeof (WCHAR) && iswalpha ((p)->Buffer[0]) \
- && (p)->Buffer[1] == L':' && \
- ((p)->Length == 2 * sizeof (WCHAR) || iswdirsep ((p)->Buffer[2])))))
-
-#define iswabspath(p) \
- (iswdirsep (*(p)) || (iswalpha (*(p)) && (p)[1] == L':' && (!(p)[2] || iswdirsep ((p)[2]))))
-
#define isabspath(p) \
(isdirsep (*(p)) || (isalpha (*(p)) && (p)[1] == ':' && (!(p)[2] || isdirsep ((p)[2]))))
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] Cygwin: introduce isabspath_strict macro
2021-11-10 20:32 [PATCH 0/2] Fix a bad case of absolute path handling corinna-cygwin
2021-11-10 20:32 ` [PATCH 1/2] Cygwin: drop unused isabspath_u and iswabspath macros corinna-cygwin
@ 2021-11-10 20:32 ` corinna-cygwin
2021-11-10 22:22 ` [PATCH 0/2] Fix a bad case of absolute path handling Ken Brown
2 siblings, 0 replies; 7+ messages in thread
From: corinna-cygwin @ 2021-11-10 20:32 UTC (permalink / raw)
To: cygwin-patches
From: Corinna Vinschen <corinna@vinschen.de>
isabspath handles a path "X:", without trailing slas or backslash,
as absolute path. This breaks some scenarios with relative paths
starting with "X:". For instance, fstatat will mishandle a call
with valid dirfd and "c:" as path.
The reason is that gen_full_path_at() will check for isabspath("C:")
which returns true. So the path will be used verbatim in fstatat,
rather than being converted to a path "<dirfd-path>/c:".
So, introduce isabspath_strict, which returns true for paths starting
with "X:" only if the next char is actually a slash or backslash.
Use it from gen_full_path_at().
This still fixes only half the problem. The right thing would have been
to disallow using DOS paths in the first place. Unfortunately it's much
too late for that.
Addresses: https://cygwin.com/pipermail/cygwin/2021-November/249837.html
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
winsup/cygwin/syscalls.cc | 2 +-
winsup/cygwin/winsup.h | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 7a48e422e8f4..661c143479e4 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4714,7 +4714,7 @@ gen_full_path_at (char *path_ret, int dirfd, const char *pathname,
return -1;
}
}
- if (pathname && isabspath (pathname))
+ if (pathname && isabspath_strict (pathname))
stpcpy (path_ret, pathname);
else
{
diff --git a/winsup/cygwin/winsup.h b/winsup/cygwin/winsup.h
index f6fea6313d56..1f265ec28934 100644
--- a/winsup/cygwin/winsup.h
+++ b/winsup/cygwin/winsup.h
@@ -139,9 +139,17 @@ extern int cygserver_running;
#undef issep
#define issep(ch) (strchr (" \t\n\r", (ch)) != NULL)
+/* Treats "X:" as absolute path.
+ FIXME: We should drop the notion that "X:" is a valid absolute path.
+ Only "X:/" and "X:\\" should be (see isabspath_strict below). The
+ problem is to find out if we have code depending on this behaviour. */
#define isabspath(p) \
(isdirsep (*(p)) || (isalpha (*(p)) && (p)[1] == ':' && (!(p)[2] || isdirsep ((p)[2]))))
+/* Treats "X:/" and "X:\\" as absolute paths, but not "X:" */
+#define isabspath_strict(p) \
+ (isdirsep (*(p)) || (isalpha (*(p)) && (p)[1] == ':' && isdirsep ((p)[2])))
+
/******************** Initialization/Termination **********************/
class per_process;
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Fix a bad case of absolute path handling
2021-11-10 20:32 [PATCH 0/2] Fix a bad case of absolute path handling corinna-cygwin
2021-11-10 20:32 ` [PATCH 1/2] Cygwin: drop unused isabspath_u and iswabspath macros corinna-cygwin
2021-11-10 20:32 ` [PATCH 2/2] Cygwin: introduce isabspath_strict macro corinna-cygwin
@ 2021-11-10 22:22 ` Ken Brown
2021-11-11 9:47 ` Corinna Vinschen
2021-11-12 16:30 ` Brian Inglis
2 siblings, 2 replies; 7+ messages in thread
From: Ken Brown @ 2021-11-10 22:22 UTC (permalink / raw)
To: cygwin-patches
On 11/10/2021 3:32 PM, corinna-cygwin@cygwin.com wrote:
> From: Corinna Vinschen <corinna@vinschen.de>
>
> As I told Takashi in PM, I will try to more often send patches to the
> cygwin-patches ML before pushing them, so there's a chance to chime in.
LGTM.
> This patch series is supposed to address the `rm -rf' problem reported
> in https://cygwin.com/pipermail/cygwin/2021-November/249837.html
>
> It was always frustrating, having to allow DOS drive letter paths for
> backward compatibility. This here is another case of ambiguity,
> triggered by the `isabspath' macro handling "X:" as absolute path, even
> without the trailing slash or backslash.
>
> Check out the 2nd patch for a more detailed description.
>
> While at it, I wonder if we might have a chance to fix these ambiguities
> in a better way. For instance, consider this:
>
> $ mkdir -p test/c:
> $ cd test
>
> As non-admin:
>
> $ touch c:/foo
> touch: cannot touch 'c:/foo': Permission denied
>
> As admin, even worse:
>
> $ touch c:/foo
> $ ls /cygdrive/c/foo
> foo
>
> As long as we support DOS paths as input, I have a hard time to see how
> to fix this, but maybe we can at least minimize the ambiguity somehow.
I can't immediately think of anything. But is it really impossible to phase out
DOS path support over a period of time? We could start with a HEADS-UP, asking
for comments, then a deprecation announcement, then something like the old
dosfilewarning option, then a more forceful warning that can't be turned off,
and finally removal of support. This could be done over a period of several
years (not sure how many).
We could also put lines like
# C:/ on /c type ntfs (binary,posix=0)
into the default /etc/fstab.
Ken
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Fix a bad case of absolute path handling
2021-11-10 22:22 ` [PATCH 0/2] Fix a bad case of absolute path handling Ken Brown
@ 2021-11-11 9:47 ` Corinna Vinschen
2021-11-11 14:10 ` Ken Brown
2021-11-12 16:30 ` Brian Inglis
1 sibling, 1 reply; 7+ messages in thread
From: Corinna Vinschen @ 2021-11-11 9:47 UTC (permalink / raw)
To: cygwin-patches
On Nov 10 17:22, Ken Brown wrote:
> On 11/10/2021 3:32 PM, corinna-cygwin@cygwin.com wrote:
> > From: Corinna Vinschen <corinna@vinschen.de>
> >
> > As I told Takashi in PM, I will try to more often send patches to the
> > cygwin-patches ML before pushing them, so there's a chance to chime in.
>
> LGTM.
>
> > This patch series is supposed to address the `rm -rf' problem reported
> > in https://cygwin.com/pipermail/cygwin/2021-November/249837.html
> >
> > It was always frustrating, having to allow DOS drive letter paths for
> > backward compatibility. This here is another case of ambiguity,
> > triggered by the `isabspath' macro handling "X:" as absolute path, even
> > without the trailing slash or backslash.
> >
> > Check out the 2nd patch for a more detailed description.
> >
> > While at it, I wonder if we might have a chance to fix these ambiguities
> > in a better way. For instance, consider this:
> >
> > $ mkdir -p test/c:
> > $ cd test
> >
> > As non-admin:
> >
> > $ touch c:/foo
> > touch: cannot touch 'c:/foo': Permission denied
> >
> > As admin, even worse:
> >
> > $ touch c:/foo
> > $ ls /cygdrive/c/foo
> > foo
> >
> > As long as we support DOS paths as input, I have a hard time to see how
> > to fix this, but maybe we can at least minimize the ambiguity somehow.
>
> I can't immediately think of anything. But is it really impossible to phase
> out DOS path support over a period of time? We could start with a HEADS-UP,
> asking for comments, then a deprecation announcement, then something like
> the old dosfilewarning option, then a more forceful warning that can't be
> turned off, and finally removal of support. This could be done over a
> period of several years (not sure how many).
Yeah, we might try again. Just not over years, we'll probably lose
track over time. I'd appreciate a shorter period with a chance to see
the end.
The problem is that people are probably using DOS paths all the time.
Makefiles and scripts mixing Cygwin and DOS tools come to mind.
For the time being, I wonder if we could start with isabspath being
always strict so at least X: isn't an abspath at all.
>
> We could also put lines like
>
> # C:/ on /c type ntfs (binary,posix=0)
>
> into the default /etc/fstab.
Commented out, you mean? Just as hint? We could do that. Personally I
don't like these shortcuts, I rather use a shorter cygdrive prefix, like
/mnt, but I see how this could convince people. Scripts with mixed
tools will always be a problem, though.
Corinna
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Fix a bad case of absolute path handling
2021-11-11 9:47 ` Corinna Vinschen
@ 2021-11-11 14:10 ` Ken Brown
0 siblings, 0 replies; 7+ messages in thread
From: Ken Brown @ 2021-11-11 14:10 UTC (permalink / raw)
To: cygwin-patches
On 11/11/2021 4:47 AM, Corinna Vinschen wrote:
> On Nov 10 17:22, Ken Brown wrote:
>> I can't immediately think of anything. But is it really impossible to phase
>> out DOS path support over a period of time? We could start with a HEADS-UP,
>> asking for comments, then a deprecation announcement, then something like
>> the old dosfilewarning option, then a more forceful warning that can't be
>> turned off, and finally removal of support. This could be done over a
>> period of several years (not sure how many).
>
> Yeah, we might try again. Just not over years, we'll probably lose
> track over time. I'd appreciate a shorter period with a chance to see
> the end.
>
> The problem is that people are probably using DOS paths all the time.
> Makefiles and scripts mixing Cygwin and DOS tools come to mind.
So maybe an RFC email would be useful rather than a HEADS-UP, just to see how
widespread it is.
> For the time being, I wonder if we could start with isabspath being
> always strict so at least X: isn't an abspath at all.
Sounds reasonable.
>> We could also put lines like
>>
>> # C:/ on /c type ntfs (binary,posix=0)
>>
>> into the default /etc/fstab.
>
> Commented out, you mean? Just as hint?
Yes.
> We could do that. Personally I
> don't like these shortcuts, I rather use a shorter cygdrive prefix, like
> /mnt, but I see how this could convince people. Scripts with mixed
> tools will always be a problem, though.
Ken
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Fix a bad case of absolute path handling
2021-11-10 22:22 ` [PATCH 0/2] Fix a bad case of absolute path handling Ken Brown
2021-11-11 9:47 ` Corinna Vinschen
@ 2021-11-12 16:30 ` Brian Inglis
1 sibling, 0 replies; 7+ messages in thread
From: Brian Inglis @ 2021-11-12 16:30 UTC (permalink / raw)
To: cygwin-patches
On 2021-11-10 15:22, Ken Brown wrote:
> On 11/10/2021 3:32 PM, corinna-cygwin@cygwin.com wrote:
>> From: Corinna Vinschen <corinna@vinschen.de>
>>
>> As I told Takashi in PM, I will try to more often send patches to the
>> cygwin-patches ML before pushing them, so there's a chance to chime in.
>
> LGTM.
>
>> This patch series is supposed to address the `rm -rf' problem reported
>> in https://cygwin.com/pipermail/cygwin/2021-November/249837.html
>>
>> It was always frustrating, having to allow DOS drive letter paths for
>> backward compatibility. This here is another case of ambiguity,
>> triggered by the `isabspath' macro handling "X:" as absolute path, even
>> without the trailing slash or backslash.
>>
>> Check out the 2nd patch for a more detailed description.
>>
>> While at it, I wonder if we might have a chance to fix these ambiguities
>> in a better way. For instance, consider this:
>>
>> $ mkdir -p test/c:
>> $ cd test
>>
>> As non-admin:
>>
>> $ touch c:/foo
>> touch: cannot touch 'c:/foo': Permission denied
>>
>> As admin, even worse:
>>
>> $ touch c:/foo
>> $ ls /cygdrive/c/foo
>> foo
>>
>> As long as we support DOS paths as input, I have a hard time to see how
>> to fix this, but maybe we can at least minimize the ambiguity somehow.
>
> I can't immediately think of anything. But is it really impossible to
> phase out DOS path support over a period of time? We could start with a
> HEADS-UP, asking for comments, then a deprecation announcement, then
> something like the old dosfilewarning option, then a more forceful
> warning that can't be turned off, and finally removal of support. This
> could be done over a period of several years (not sure how many).
>
> We could also put lines like
>
> # C:/ on /c type ntfs (binary,posix=0)
>
> into the default /etc/fstab.
NO! BTDT GTS.
Try getting help from any DOS/cmd type command or subcommand.
Shell expands /? to list of all mapped drives /c /d ... /s /v /y which
gives you a bunch of potentially destructive switches.
--
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] 7+ messages in thread
end of thread, other threads:[~2021-11-12 16:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 20:32 [PATCH 0/2] Fix a bad case of absolute path handling corinna-cygwin
2021-11-10 20:32 ` [PATCH 1/2] Cygwin: drop unused isabspath_u and iswabspath macros corinna-cygwin
2021-11-10 20:32 ` [PATCH 2/2] Cygwin: introduce isabspath_strict macro corinna-cygwin
2021-11-10 22:22 ` [PATCH 0/2] Fix a bad case of absolute path handling Ken Brown
2021-11-11 9:47 ` Corinna Vinschen
2021-11-11 14:10 ` Ken Brown
2021-11-12 16:30 ` Brian Inglis
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).