Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Matheus Castanho <msc at linux dot ibm dot com>]

Hi Florian,

Some considerations on the test code below.

On 2/12/20 5:24 PM, Florian Weimer wrote:
> 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                    | 83 ++++++++++++--------------------------
>  sysdeps/unix/sysv/linux/fchmodat.c | 28 +++++++++++--
>  2 files changed, 49 insertions(+), 62 deletions(-)
> 
> diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
> index 73e45549af..59873f130d 100644
> --- a/io/tst-lchmod.c
> +++ b/io/tst-lchmod.c
> @@ -102,68 +102,44 @@ 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, 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.

>    xstat (path_file, &st);
>    TEST_COMPARE (st.st_mode & 0777, 2);
> +  xlstat (path_to_file, &st);
> +  TEST_COMPARE (original_symlink_mode, st.st_mode);

Ok.

> +  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);

Ok.

> +
> +  /* Changing the mode of a symbolic link should fail fail.  */

Duplicated 'fail'.

>    arg = select_path (do_relative_path, path_to_file, "to-file");
> -  TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0);
> +  int ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
> +  TEST_COMPARE (ret, -1);
> +  TEST_COMPARE (errno, EOPNOTSUPP);

Ok.

> +
> +  /* 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_mod
Ok.

>  
> -  /* 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?

>  
> -      /* 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);
> -
> -    }
> -  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);
> -    }
> -
> -  /* If we have NOFOLLOW support, we should be able to change the mode
> -     of a dangling symbolic linqk 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 +154,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);

Ok.

>      }
>  
>     /* 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)
>  	{
> 

--
Matheus Castanho