This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths
On 2/18/20 11:50 AM, Florian Weimer wrote:
> * Matheus Castanho:
>
>> Some considerations on the test code below.
>
> Thanks.
>
>>> + arg = select_path (do_relative_path, path_to_file, "to-file");
>>> + TEST_COMPARE (chmod_func (fd, path_to_file, 2, 0), 0);
>>
>> Shouldn't you use arg instead of path_to_file here? Like
>> TEST_COMPARE (chmod_func (fd, arg, 2, 0), 0);
>>
>> Otherwise the select_path call is not needed.
>
> Fixed to use arg.
>
>>> +
>>> + /* Changing the mode of a symbolic link should fail fail. */
>>
>> Duplicated 'fail'.
>
> Fixed.
>
>>> - /* Changing the mode of a symbolic link may fail. */
>>> arg = select_path (do_relative_path, path_to_file, "to-file");
>>> ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
>>> - if (nofollow)
>>> - {
>>> - TEST_COMPARE (ret, 0);
>>> + TEST_COMPARE (ret, -1);
>>> + TEST_COMPARE (errno, EOPNOTSUPP);
>>
>> This is exactly the same test as above. Is it really needed?
>
> Agreed, I've dropped it.
>
> New patch below.
>
> Florian
>
> 8<------------------------------------------------------------------8<
> It appears that the ability to change symbolic link modes through such
> paths is unintended. On several file systems, the operation fails with
> EOPNOTSUPP, even though the symbolic link permissions are updated.
> The expected behavior is a failure to update the permissions, without
> file system changes.
>
> -----
> io/tst-lchmod.c | 80 ++++++++++----------------------------
> sysdeps/unix/sysv/linux/fchmodat.c | 28 +++++++++++--
> 2 files changed, 45 insertions(+), 63 deletions(-)
>
> diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
> index 73e45549af..33ca474b50 100644
> --- a/io/tst-lchmod.c
> +++ b/io/tst-lchmod.c
> @@ -102,68 +102,39 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
> TEST_VERIFY ((st.st_mode & 0777) != 2);
> mode_t original_symlink_mode = st.st_mode;
>
> - /* Set to true if AT_SYMLINK_NOFOLLOW is supported. */
> - bool nofollow;
> -
> /* We should be able to change the mode of a file, including through
> the symbolic link to-file. */
> const char *arg = select_path (do_relative_path, path_file, "file");
> TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
> xstat (path_file, &st);
> TEST_COMPARE (st.st_mode & 0777, 1);
> - int ret = chmod_func (fd, path_file, 2, AT_SYMLINK_NOFOLLOW);
> - if (ret == 0)
> - {
> - printf ("info: AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
> - nofollow = true;
> - }
> - else
> - {
> - printf ("info: no AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
> - nofollow = false;
> -
> - /* Set up things for the code below. */
> - TEST_COMPARE (chmod_func (fd, path_file, 2, 0), 0);
> - }
> + arg = select_path (do_relative_path, path_to_file, "to-file");
> + TEST_COMPARE (chmod_func (fd, arg, 2, 0), 0);
> xstat (path_file, &st);
> TEST_COMPARE (st.st_mode & 0777, 2);
> - arg = select_path (do_relative_path, path_to_file, "to-file");
> - TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0);
> + xlstat (path_to_file, &st);
> + TEST_COMPARE (original_symlink_mode, st.st_mode);
> + arg = select_path (do_relative_path, path_file, "file");
> + TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
> xstat (path_file, &st);
> TEST_COMPARE (st.st_mode & 0777, 1);
> xlstat (path_to_file, &st);
> TEST_COMPARE (original_symlink_mode, st.st_mode);
>
> - /* Changing the mode of a symbolic link may fail. */
> + /* Changing the mode of a symbolic link should fail. */
> arg = select_path (do_relative_path, path_to_file, "to-file");
> - ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
> - if (nofollow)
> - {
> - TEST_COMPARE (ret, 0);
> -
> - /* The mode of the link changed. */
> - xlstat (path_to_file, &st);
> - TEST_COMPARE (st.st_mode & 0777, 2);
> -
> - /* But the mode of the file is unchanged. */
> - xstat (path_file, &st);
> - TEST_COMPARE (st.st_mode & 0777, 1);
> + int ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
> + TEST_COMPARE (ret, -1);
> + TEST_COMPARE (errno, EOPNOTSUPP);
>
> - }
> - else
> - {
> - TEST_COMPARE (ret, -1);
> - TEST_COMPARE (errno, EOPNOTSUPP);
> -
> - /* The modes should remain unchanged. */
> - xstat (path_file, &st);
> - TEST_COMPARE (st.st_mode & 0777, 1);
> - xlstat (path_to_file, &st);
> - TEST_COMPARE (original_symlink_mode, st.st_mode);
> - }
> + /* The modes should remain unchanged. */
> + xstat (path_file, &st);
> + TEST_COMPARE (st.st_mode & 0777, 1);
> + xlstat (path_to_file, &st);
> + TEST_COMPARE (original_symlink_mode, st.st_mode);
>
> - /* If we have NOFOLLOW support, we should be able to change the mode
> - of a dangling symbolic link or a symbolic link loop. */
> + /* Likewise, changing dangling and looping symbolic links must
> + fail. */
> const char *paths[] = { path_dangling, path_loop };
> for (size_t i = 0; i < array_length (paths); ++i)
> {
> @@ -178,19 +149,10 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
> original_symlink_mode = st.st_mode;
> arg = select_path (do_relative_path, path, filename);
> ret = chmod_func (fd, arg, new_mode, AT_SYMLINK_NOFOLLOW);
> - if (nofollow)
> - {
> - TEST_COMPARE (ret, 0);
> - xlstat (path, &st);
> - TEST_COMPARE (st.st_mode & 0777, new_mode);
> - }
> - else /* !nofollow. */
> - {
> - TEST_COMPARE (ret, -1);
> - TEST_COMPARE (errno, EOPNOTSUPP);
> - xlstat (path, &st);
> - TEST_COMPARE (st.st_mode, original_symlink_mode);
> - }
> + TEST_COMPARE (ret, -1);
> + TEST_COMPARE (errno, EOPNOTSUPP);
> + xlstat (path, &st);
> + TEST_COMPARE (st.st_mode, original_symlink_mode);
> }
>
> /* A missing file should always result in ENOENT. The presence of
> diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
> index 719053b333..17eca54051 100644
> --- a/sysdeps/unix/sysv/linux/fchmodat.c
> +++ b/sysdeps/unix/sysv/linux/fchmodat.c
> @@ -45,6 +45,30 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
> caller can treat them as temporary if necessary. */
> return pathfd;
>
> + /* Use fstatat because fstat does not work on O_PATH descriptors
> + before Linux 3.6. */
> + struct stat64 st;
> + if (fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
> + {
> + __close_nocancel (pathfd);
> + return -1;
> + }
> +
> + /* Some Linux versions with some file systems can actually
> + change symbolic link permissions via /proc, but this is not
> + intentional, and it gives inconsistent results (e.g., error
> + return despite mode change). The expected behavior is that
> + symbolic link modes cannot be changed at all, and this check
> + enforces that. */
> + if (S_ISLNK (st.st_mode))
> + {
> + __close_nocancel (pathfd);
> + __set_errno (EOPNOTSUPP);
> + return -1;
> + }
> +
> + /* For most file systems, fchmod does not operate on O_PATH
> + descriptors, so go through /proc. */
> char buf[32];
> if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
> {
> @@ -54,10 +78,6 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
> 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)
> {
>
LGTM.
Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
--
Matheus Castanho