[3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]
Commit Message
/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(-)
Comments
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.
* 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.
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.
* 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.
@@ -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)