[3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

Message ID 3391fd4d8a6c8d6457985665b1b824dddf64e422.1579723048.git.fweimer@redhat.com
State New
Headers show
Series
  • Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat
Related show

Commit Message

Florian Weimer Jan. 22, 2020, 8:03 p.m.
/proc/self/fd files are special and chmod on O_PATH descriptors
in that directory operates on the symbolic link itself (like lchmod).
---
 sysdeps/unix/sysv/linux/fchmodat.c | 61 +++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 10 deletions(-)

-- 
2.24.1

Comments

Paul Eggert Feb. 9, 2020, 8:30 a.m. | #1
On 1/22/20 12:03 PM, Florian Weimer wrote:
> +	  if (errno == ENFILE || errno == EMFILE)

> +	    /* These errors cannot happen with a straight fchmodat

> +	       operation because it does not create file descriptors,

> +	       so hide them.  */

> +	    __set_errno (EOPNOTSUPP);

> +	  /* Otherwise, this should accurately reflect the expected

> +	     error from fchmodat (e.g., EBADF or ENOENT).  */


These lines can be omitted. I omitted them when recently adding similar code to 
Gnulib in <https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/fchmodat.c>. 
There's no need for fchmodat to masquerade ENFILE and EMFILE as EOPNOTSUPP: 
POSIX does not require it and masquerading those two errno values would be 
misleading, in the sense that it would make it harder for the caller to diagnose 
what actually went wrong.

> +      char buf[32];

> +      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)

> +	{

> +	  __close_nocancel (pathfd);

> +	  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOPNOTSUPP);

> +	}


Similarly here. Also, there should be no reason for snprintf to fail. One 
possibility is to replace this with what's in Gnulib:

   static char const fmt[] = "/proc/self/fd/%d";
   char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
   sprintf (buf, fmt, pathfd);

where INT_BUFSIZE_BOUND is taken from <intprops.h>.

Otherwise, this patch series looks OK to me; thanks for writing it.
Florian Weimer Feb. 9, 2020, 8:57 a.m. | #2
* Paul Eggert:

> On 1/22/20 12:03 PM, Florian Weimer wrote:

>> +	  if (errno == ENFILE || errno == EMFILE)

>> +	    /* These errors cannot happen with a straight fchmodat

>> +	       operation because it does not create file descriptors,

>> +	       so hide them.  */

>> +	    __set_errno (EOPNOTSUPP);

>> +	  /* Otherwise, this should accurately reflect the expected

>> +	     error from fchmodat (e.g., EBADF or ENOENT).  */

>

> These lines can be omitted. I omitted them when recently adding similar code to 

> Gnulib in <https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/fchmodat.c>. 

> There's no need for fchmodat to masquerade ENFILE and EMFILE as EOPNOTSUPP: 

> POSIX does not require it and masquerading those two errno values would be 

> misleading, in the sense that it would make it harder for the caller to diagnose 

> what actually went wrong.


Hmm.  The error code is really obscure, particularly with AT_FDCWD,
where there is no file descriptor involved.  I hope that we can
eventually make it go away with proper kernel support.  But I see your
point about telling transient errors (such as ENOMEM) from more
persistent ones.

>> +      char buf[32];

>> +      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)

>> +	{

>> +	  __close_nocancel (pathfd);

>> +	  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOPNOTSUPP);

>> +	}

>

> Similarly here. Also, there should be no reason for snprintf to fail. One 

> possibility is to replace this with what's in Gnulib:

>

>    static char const fmt[] = "/proc/self/fd/%d";

>    char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];

>    sprintf (buf, fmt, pathfd);


I think we should add a proper wrapper for this eventually, where the
buffer allocation in the caller is properly abstracted via a
suitable struct type.

> where INT_BUFSIZE_BOUND is taken from <intprops.h>.

>

> Otherwise, this patch series looks OK to me; thanks for writing it.


Thanks.

What do you think about introducing new symbol versions vs keeping
things as they are in the patch today?

If we do not introduce new symbol versions, we should strive to
backport this change widely, so that users do not run into obscure
failures.
Paul Eggert Feb. 9, 2020, 10:06 a.m. | #3
On 2/9/20 12:57 AM, Florian Weimer wrote:
> What do you think about introducing new symbol versions vs keeping

> things as they are in the patch today?


I'm not quite following the relationship between the two alternatives. Isn't the 
symbol version independent of whether we keep things as they are now, or install 
the patch that you proposed, or install that patch with my further suggestions? 
That is, I don't see why the patch (or patch variant) would require us to change 
symbol versions.

> If we do not introduce new symbol versions, we should strive to

> backport this change widely, so that users do not run into obscure

> failures.


Yes, backporting would be good. Essentially Gnulib is already doing that, for 
Gnulib-using apps.
Florian Weimer Feb. 9, 2020, 10:32 a.m. | #4
* Paul Eggert:

> On 2/9/20 12:57 AM, Florian Weimer wrote:

>> What do you think about introducing new symbol versions vs keeping

>> things as they are in the patch today?

>

> I'm not quite following the relationship between the two

> alternatives. Isn't the symbol version independent of whether we

> keep things as they are now, or install the patch that you proposed,

> or install that patch with my further suggestions?  That is, I don't

> see why the patch (or patch variant) would require us to change

> symbol versions.


If these functions are now going to be used widely, introducing a new
symbol version at the time of the bug fix ensures that applications do
not accidentally run on glibc versions which lack this change, where
they might fail in obscure ways.
Florian Weimer Feb. 11, 2020, 3:25 p.m. | #5
* Paul Eggert:

> On 2/9/20 12:57 AM, Florian Weimer wrote:

>> What do you think about introducing new symbol versions vs keeping

>> things as they are in the patch today?

>

> I'm not quite following the relationship between the two

> alternatives. Isn't the symbol version independent of whether we keep

> things as they are now, or install the patch that you proposed, or

> install that patch with my further suggestions? That is, I don't see

> why the patch (or patch variant) would require us to change symbol

> versions.

>

>> If we do not introduce new symbol versions, we should strive to

>> backport this change widely, so that users do not run into obscure

>> failures.

>

> Yes, backporting would be good. Essentially Gnulib is already doing

> that, for Gnulib-using apps.


Okay, this is what I'm going to commit soon, with no symbol version
changes.

Thanks,
Florian

8<------------------------------------------------------------------8<
/proc/self/fd files are special and chmod on O_PATH descriptors
in that directory operates on the symbolic link itself (like lchmod).

-----
 sysdeps/unix/sysv/linux/fchmodat.c | 57 +++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index c41ebb290d..719053b333 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -18,24 +18,61 @@
 
 #include <errno.h>
 #include <fcntl.h>
-#include <stddef.h>
+#include <not-cancel.h>
 #include <stdio.h>
-#include <string.h>
-#include <unistd.h>
+#include <sys/stat.h>
 #include <sys/types.h>
-#include <alloca.h>
 #include <sysdep.h>
+#include <unistd.h>
 
 int
 fchmodat (int fd, const char *file, mode_t mode, int flag)
 {
-  if (flag & ~AT_SYMLINK_NOFOLLOW)
+  if (flag == 0)
+    return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+  else if (flag != AT_SYMLINK_NOFOLLOW)
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-#ifndef __NR_lchmod		/* Linux so far has no lchmod syscall.  */
-  if (flag & AT_SYMLINK_NOFOLLOW)
-    return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
-#endif
+  else
+    {
+      /* The kernel system call does not have a mode argument.
+	 However, we can create an O_PATH descriptor and change that
+	 via /proc (which does not resolve symbolic links).  */
 
-  return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+      int pathfd = __openat_nocancel (fd, file,
+				      O_PATH | O_NOFOLLOW | O_CLOEXEC);
+      if (pathfd < 0)
+	/* This may report errors such as ENFILE and EMFILE.  The
+	   caller can treat them as temporary if necessary.  */
+	return pathfd;
+
+      char buf[32];
+      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
+	{
+	  /* This also may report strange error codes to the caller
+	     (although snprintf really should not fail).  */
+	  __close_nocancel (pathfd);
+	  return -1;
+	}
+
+      /* This operates directly on the symbolic link if it is one.
+	 /proc/self/fd files look like symbolic links, but they are
+	 not.  (fchmod and fchmodat do not work on O_PATH descriptors,
+	 similar to fstat before Linux 3.6.)  */
+      int ret = __chmod (buf, mode);
+      if (ret != 0)
+	{
+	  if (errno == ENOENT)
+	    /* /proc has not been mounted.  Without /proc, there is no
+	       way to upgrade the O_PATH descriptor to a full
+	       descriptor.  It is also not possible to re-open the
+	       file without O_PATH because the file name may refer to
+	       another file, and opening that without O_PATH may have
+	       side effects (such as blocking, device rewinding, or
+	       releasing POSIX locks).  */
+	    __set_errno (EOPNOTSUPP);
+	}
+      __close_nocancel (pathfd);
+      return ret;
+    }
 }
 libc_hidden_def (fchmodat)
Paul Eggert Feb. 12, 2020, 1:36 a.m. | #6
Thanks for doing that; looks good.

Patch

diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index c41ebb290d..ac318ceb79 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -18,24 +18,65 @@ 
 
 #include <errno.h>
 #include <fcntl.h>
-#include <stddef.h>
+#include <not-cancel.h>
 #include <stdio.h>
-#include <string.h>
-#include <unistd.h>
+#include <sys/stat.h>
 #include <sys/types.h>
-#include <alloca.h>
 #include <sysdep.h>
+#include <unistd.h>
 
 int
 fchmodat (int fd, const char *file, mode_t mode, int flag)
 {
-  if (flag & ~AT_SYMLINK_NOFOLLOW)
+  if (flag == 0)
+    return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+  else if (flag != AT_SYMLINK_NOFOLLOW)
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-#ifndef __NR_lchmod		/* Linux so far has no lchmod syscall.  */
-  if (flag & AT_SYMLINK_NOFOLLOW)
-    return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
-#endif
+  else
+    {
+      /* The kernel system call does not have a mode argument.
+	 However, we can create an O_PATH descriptor and change that
+	 via /proc (which does not resolve symbolic links).  */
+
+      int pathfd = __openat_nocancel (fd, file,
+				      O_PATH | O_NOFOLLOW | O_CLOEXEC);
+      if (pathfd < 0)
+	{
+	  if (errno == ENFILE || errno == EMFILE)
+	    /* These errors cannot happen with a straight fchmodat
+	       operation because it does not create file descriptors,
+	       so hide them.  */
+	    __set_errno (EOPNOTSUPP);
+	  /* Otherwise, this should accurately reflect the expected
+	     error from fchmodat (e.g., EBADF or ENOENT).  */
+	  return pathfd;
+	}
+
+      char buf[32];
+      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
+	{
+	  __close_nocancel (pathfd);
+	  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOPNOTSUPP);
+	}
 
-  return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+      /* This operates directly on the symbolic link if it is one.
+	 /proc/self/fd files look like symbolic links, but they are
+	 not.  (fchmod and fchmodat do not work on O_PATH descriptors,
+	 similar to fstat before Linux 3.6.)  */
+      int ret = __chmod (buf, mode);
+      if (ret != 0)
+	{
+	  if (errno == ENOENT)
+	    /* /proc has not been mounted.  In general, we cannot use
+	       openat with AT_EMPTY_PATH to upgrade the descriptor
+	       because we may not have permission to open the file,
+	       and opening files and closing them again may have side
+	       effects (such as rewinding tape devices, or releasing
+	       POSIX locks).  */
+	    __set_errno (EOPNOTSUPP);
+	}
+      __close_nocancel (pathfd);
+      return ret;
+    }
 }
 libc_hidden_def (fchmodat)