Re: [PATCH 0/2] Fix a bad case of absolute path handling
On 11/10/2021 3:32 PM, corinna-cyg...@cygwin.com wrote: From: Corinna Vinschen 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
[PATCH 2/2] Cygwin: introduce isabspath_strict macro
From: Corinna Vinschen
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 "/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
---
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
[PATCH 0/2] Fix a bad case of absolute path handling
From: Corinna Vinschen 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
[PATCH 1/2] Cygwin: drop unused isabspath_u and iswabspath macros
From: Corinna Vinschen
Signed-off-by: Corinna Vinschen
---
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
Re: [PATCH] Cygwin: pipe: Handle WAIT_CANCELED when waiting read_mtx.
On Nov 10 17:23, Takashi Yano wrote:
> - Add missing handling for WAIT_CANCELED in cygwait() for read_mtx
> in raw_read().
> ---
> winsup/cygwin/fhandler_pipe.cc | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> index bc06d157c..13731437e 100644
> --- a/winsup/cygwin/fhandler_pipe.cc
> +++ b/winsup/cygwin/fhandler_pipe.cc
> @@ -302,10 +302,18 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>set_errno (EAGAIN);
>len = (size_t) -1;
>return;
> -default:
> +case WAIT_SIGNALED:
>set_errno (EINTR);
>len = (size_t) -1;
>return;
> +case WAIT_CANCELED:
> + pthread::static_cancel_self ();
> + /* NOTREACHED */
> +default:
> + /* Should not reach here. */
> + __seterrno ();
> + len = (size_t) -1;
> + return;
> }
>while (nbytes < len)
> {
> --
> 2.33.0
ACK. Please push.
Thanks,
Corinna
[PATCH] Cygwin: pipe: Handle WAIT_CANCELED when waiting read_mtx.
- Add missing handling for WAIT_CANCELED in cygwait() for read_mtx
in raw_read().
---
winsup/cygwin/fhandler_pipe.cc | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index bc06d157c..13731437e 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -302,10 +302,18 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
set_errno (EAGAIN);
len = (size_t) -1;
return;
-default:
+case WAIT_SIGNALED:
set_errno (EINTR);
len = (size_t) -1;
return;
+case WAIT_CANCELED:
+ pthread::static_cancel_self ();
+ /* NOTREACHED */
+default:
+ /* Should not reach here. */
+ __seterrno ();
+ len = (size_t) -1;
+ return;
}
while (nbytes < len)
{
--
2.33.0
