[PATCH v2] Cygwin: rmdir: fail if last component is a symlink, as on Linux

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] Cygwin: rmdir: fail if last component is a symlink, as on Linux

Ken Brown-6
If the last component of the directory name is a symlink followed by a
slash, rmdir should fail, even if the symlink resolves to an existing
empty directory.

mkdir was similarly fixed in 2009 in commit
52dba6a5c45e8d8ba1e237a15213311dc11d91fb.  Modify a comment to clarify
the purpose of that commit.

Addresses https://cygwin.com/ml/cygwin/2019-09/msg00221.html.
---
 winsup/cygwin/dir.cc | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index b757851d5..0e0535891 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -305,15 +305,14 @@ mkdir (const char *dir, mode_t mode)
 
   __try
     {
-      /* POSIX says mkdir("symlink-to-missing/") should create the
- directory "missing", but Linux rejects it with EEXIST.  Copy
- Linux behavior for now.  */
-
       if (!*dir)
  {
   set_errno (ENOENT);
   __leave;
  }
+      /* Following Linux, do not resolve the last component of DIR if
+ it is a symlink, even if DIR has a trailing slash.  Achieve
+ this by stripping trailing slashes or backslashes.  */
       if (isdirsep (dir[strlen (dir) - 1]))
  {
   /* This converts // to /, but since both give EEXIST, we're okay.  */
@@ -351,9 +350,29 @@ rmdir (const char *dir)
 {
   int res = -1;
   fhandler_base *fh = NULL;
+  tmp_pathbuf tp;
 
   __try
     {
+      if (!*dir)
+ {
+  set_errno (ENOENT);
+  __leave;
+ }
+
+      /* Following Linux, do not resolve the last component of DIR if
+ it is a symlink, even if DIR has a trailing slash.  Achieve
+ this by stripping trailing slashes or backslashes.  */
+      if (isdirsep (dir[strlen (dir) - 1]))
+ {
+  /* This converts // to /, but since both give ENOTEMPTY,
+     we're okay.  */
+  char *buf;
+  char *p = stpcpy (buf = tp.c_get (), dir) - 1;
+  dir = buf;
+  while (p > dir && isdirsep (*p))
+    *p-- = '\0';
+ }
       if (!(fh = build_fh_name (dir, PC_SYM_NOFOLLOW)))
  __leave;   /* errno already set */;
 
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: rmdir: fail if last component is a symlink, as on Linux

Eric Blake (cygwin)-2
On 9/24/19 12:55 PM, Ken Brown wrote:

> If the last component of the directory name is a symlink followed by a
> slash, rmdir should fail, even if the symlink resolves to an existing
> empty directory.
>
> mkdir was similarly fixed in 2009 in commit
> 52dba6a5c45e8d8ba1e237a15213311dc11d91fb.  Modify a comment to clarify
> the purpose of that commit.
>
> Addresses https://cygwin.com/ml/cygwin/2019-09/msg00221.html.
> ---
>  winsup/cygwin/dir.cc | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
> index b757851d5..0e0535891 100644
> --- a/winsup/cygwin/dir.cc
> +++ b/winsup/cygwin/dir.cc
> @@ -305,15 +305,14 @@ mkdir (const char *dir, mode_t mode)
>  
>    __try
>      {
> -      /* POSIX says mkdir("symlink-to-missing/") should create the
> - directory "missing", but Linux rejects it with EEXIST.  Copy
> - Linux behavior for now.  */
> -
>        if (!*dir)
>   {
>    set_errno (ENOENT);
>    __leave;
>   }
> +      /* Following Linux, do not resolve the last component of DIR if
> + it is a symlink, even if DIR has a trailing slash.  Achieve
> + this by stripping trailing slashes or backslashes.  */
Maybe even "Following Linux, and intentionally ignoring POSIX, do not..."

> +
> +      /* Following Linux, do not resolve the last component of DIR if
> + it is a symlink, even if DIR has a trailing slash.  Achieve
> + this by stripping trailing slashes or backslashes.  */
> +      if (isdirsep (dir[strlen (dir) - 1]))
> + {
> +  /* This converts // to /, but since both give ENOTEMPTY,
> +     we're okay.  */
> +  char *buf;
> +  char *p = stpcpy (buf = tp.c_get (), dir) - 1;
> +  dir = buf;
> +  while (p > dir && isdirsep (*p))
> +    *p-- = '\0';
> + }
>        if (!(fh = build_fh_name (dir, PC_SYM_NOFOLLOW)))
>   __leave;   /* errno already set */;
>  
Looks okay to me.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: rmdir: fail if last component is a symlink, as on Linux

Ken Brown-6
On 9/24/2019 2:27 PM, Eric Blake wrote:

> On 9/24/19 12:55 PM, Ken Brown wrote:
>> If the last component of the directory name is a symlink followed by a
>> slash, rmdir should fail, even if the symlink resolves to an existing
>> empty directory.
>>
>> mkdir was similarly fixed in 2009 in commit
>> 52dba6a5c45e8d8ba1e237a15213311dc11d91fb.  Modify a comment to clarify
>> the purpose of that commit.
>>
>> Addresses https://cygwin.com/ml/cygwin/2019-09/msg00221.html.
>> ---
>>   winsup/cygwin/dir.cc | 27 +++++++++++++++++++++++----
>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
>> index b757851d5..0e0535891 100644
>> --- a/winsup/cygwin/dir.cc
>> +++ b/winsup/cygwin/dir.cc
>> @@ -305,15 +305,14 @@ mkdir (const char *dir, mode_t mode)
>>  
>>     __try
>>       {
>> -      /* POSIX says mkdir("symlink-to-missing/") should create the
>> - directory "missing", but Linux rejects it with EEXIST.  Copy
>> - Linux behavior for now.  */
>> -
>>         if (!*dir)
>>   {
>>    set_errno (ENOENT);
>>    __leave;
>>   }
>> +      /* Following Linux, do not resolve the last component of DIR if
>> + it is a symlink, even if DIR has a trailing slash.  Achieve
>> + this by stripping trailing slashes or backslashes.  */
>
> Maybe even "Following Linux, and intentionally ignoring POSIX, do not..."
>
>> +
>> +      /* Following Linux, do not resolve the last component of DIR if
>> + it is a symlink, even if DIR has a trailing slash.  Achieve
>> + this by stripping trailing slashes or backslashes.  */
>> +      if (isdirsep (dir[strlen (dir) - 1]))
>> + {
>> +  /* This converts // to /, but since both give ENOTEMPTY,
>> +     we're okay.  */
>> +  char *buf;
>> +  char *p = stpcpy (buf = tp.c_get (), dir) - 1;
>> +  dir = buf;
>> +  while (p > dir && isdirsep (*p))
>> +    *p-- = '\0';
>> + }
>>         if (!(fh = build_fh_name (dir, PC_SYM_NOFOLLOW)))
>>   __leave;   /* errno already set */;
>>  
>
> Looks okay to me.

Thanks.  Does the "intentionally ignoring POSIX" part apply to rmdir also?  I
didn't find it easy to decipher POSIX.

Even for mkdir, POSIX says, "If path names a symbolic link, mkdir() shall fail
and set errno to [EEXIST]."  See
https://pubs.opengroup.org/onlinepubs/9699919799/functions/mkdir.html#tag_16_325.

But I'm not clear on how POSIX decides whether "path names a symbolic link" in
the case where the last component is a symlink followed by a slash.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Cygwin: rmdir: fail if last component is a symlink, as on Linux

Eric Blake (cygwin)-2
On 9/24/19 2:28 PM, Ken Brown wrote:

>>
>> Looks okay to me.
>
> Thanks.  Does the "intentionally ignoring POSIX" part apply to rmdir also?  I
> didn't find it easy to decipher POSIX.
>
> Even for mkdir, POSIX says, "If path names a symbolic link, mkdir() shall fail
> and set errno to [EEXIST]."  See
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/mkdir.html#tag_16_325.
>
> But I'm not clear on how POSIX decides whether "path names a symbolic link" in
> the case where the last component is a symlink followed by a slash.
POSIX says that "/path/to/symlink" names a symlink, but
"/path/to/symlink/" (attempts to) name the directory at the contents of
the symlink.  In mkdir("/path/to/symlink/", if symlink is dangling, then
the official result is to create an empty directory at the target of the
symlink, so that symlink is no longer dangling.  In
rmdir("/path/to/symlink/"), if an empty directory exists at that
location, the directory must be removed leaving symlink dangling.

The POSIX rules are self-consistent and match what Solaris does, but it
is surprising action-at-a-distance.  Linux kernel developers have
repeatedly stated that they are unwilling to implement that behavior,
and would much rather have mkdir("/path/to/symlink/") fail with EEXIST
(the dangling symlink exists, and we can't create a directory to replace
it, nor will we create a directory to make the symlink non-dangling),
and similarly rmdir("/path/to/symlink/") fail because symlink is not a
directory, even though symlink/ resolves to a directory.

So even though we are knowingly violating POSIX, having both functions
be consistent with Linux is reasonable.  (I still hope that POSIX will
relax its stance to allow both Solaris AND Linux behaviors, but that's
not going to happen any time soon...)

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


signature.asc (499 bytes) Download Attachment