Den fre 5 jan. 2024 kl 08:45 skrev Daniel Sahlberg <
daniel.l.sahlb...@gmail.com>:
> Hi,
>
> When researching the spurious revert messages reported by Vincent Lefevre
> [1], I was looking at the code in svn_io__is_finfo_read_only() and
> svn_io_is_file_executable(). It gets the current UID and GID using the APR
> function apr_uid_current() and then compares to see if the owner of the
> file is the current user (then check for user write/execute permissions) or
> if the group owning the file is the current user's group (then check for
> group write/execute permissions) or otherwise check for world write/execute
> permissions.
>
> I think there is a failure mode when a user has write permissions to a
> file through a secondary group, something like this:
>
> [[[
> dsg@daniel-23:~/svn_trunk$ groups
> dsg adm dialout cdrom floppy sudo audio dip video plugdev netdev docker
> dsg@daniel-23:~/svn_trunk$ ls -l README
> -rw-rw-r-- 1 root sudo 2341 Sep 22 2022 README
> dsg@daniel-23:~/svn_trunk$ svn proplist -v README
> Properties on 'README':
> svn:eol-style
> native
> svn:keywords
> LastChangedDate
> dsg@daniel-23:~/svn_trunk$ svn revert README
> Reverted 'README'
> dsg@daniel-23:~/svn_trunk$ ls -l README
> -rw-rw-r-- 1 root sudo 2341 Sep 22 2022 README
> dsg@daniel-23:~/svn_trunk$ svn revert README
> Reverted 'README'
> dsg@daniel-23:~/svn_trunk$ ./subversion/svn/svn revert README
> dsg@daniel-23:~/svn_trunk$ ls -l README
> -rw-rw-r-- 1 root sudo 2341 Sep 22 2022 README
> dsg@daniel-23:~/svn_trunk$
> ]]]
>
> My user dsg has the primary group dsg and, among others, belong to the
> sudo group.
> The README file is owned by root:sudo and has 664 permission. I'm thus
> able to write to the file.
> svn revert will report Reverted, since the file README isn't owned by the
> user DSG or the group DSG and thus svn_io__is_finfo_read_only() will return
> that the file is readonly. Since the file doesn't have svn:needs-lock it
> should be RW and the Reverted message comes from Subversion trying to
> restore the W flag (which fails, thus a second svn revert call will report
> the same message).
> I initially thought about rewriting svn_io__is_finfo_read_only() to look
> also for the user's secondary groups but when asking on dev@apr.a.o if
> there is an APR way of listing a users secondary groups, Joe Orton
> suggested [2] to instead check with access(2).
>
> I've run the testsuite on Ubuntu 23.10 and OpenBSD 7.4. Initially I had an
> error on special_tests.py symlink_destination_change() which contains a
> dangling symlink. It seems the old code was checking for W on the actual
> symlink, while access() is checking on the target (which doesn't exists and
> thus ENOENT). To solve this I added the test for ENOENT in
> svn_io__is_finfo_read_only(). The same test is not needed on
> svn_io_is_file_executable() since it will not be called for a symlink (see
> revert_wc_data()). I'm not sure if this is the right way to solve this
> problem or if a change should be done in revert_wc_data() also for the
> read_only check.
>
> [[[
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1915064)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -2535,7 +2535,14 @@ svn_io__is_finfo_read_only(svn_boolean_t *read_onl
> apr_uid_t uid;
> apr_gid_t gid;
>
> - *read_only = FALSE;
> + *read_only = (access(file_info->fname, W_OK) != 0);
> + /* svn_io__is_finfo_read_only can be called with a dangling
> + * symlink. access() will check the permission on the missing
> + * target and return -1 and errno = ENOENT. Check for ENOENT
> + * and pretend the file is writeable, otherwise we will get
> + * spurious Reverted messages on the symlink.
> + */
> + if (*read_only && errno == ENOENT) *read_only = FALSE;
>
> apr_err = apr_uid_current(&uid, &gid, pool);
>
> @@ -2542,16 +2549,6 @@ svn_io__is_finfo_read_only(svn_boolean_t *read_onl
> if (apr_err)
> return svn_error_wrap_apr(apr_err, _("Error getting UID of process"));
>
> - /* Check write bit for current user. */
> - if (apr_uid_compare(uid, file_info->user) == APR_SUCCESS)
> - *read_only = !(file_info->protection & APR_UWRITE);
> -
> - else if (apr_gid_compare(gid, file_info->group) == APR_SUCCESS)
> - *read_only = !(file_info->protection & APR_GWRITE);
> -
> - else
> - *read_only = !(file_info->protection & APR_WWRITE);
> -
> #else /* WIN32 || __OS2__ || !APR_HAS_USER */
> *read_only = (file_info->protection & APR_FREADONLY);
> #endif
> @@ -2565,27 +2562,7 @@ svn_io__is_finfo_executable(svn_boolean_t *executa
> apr_pool_t *pool)
> {
> #if defined(APR_HAS_USER) && !defined(WIN32) &&!defined(__OS2__)
> - apr_status_t apr_err;
> - apr_uid_t uid;
> - apr_gid_t gid;
> -
> - *executable = FALSE;
> -
> - apr_err = apr_uid_current(&uid, &gid, pool);
> -
> - if (apr_err)
> - return svn_error_wrap_apr(apr_err, _("Error getting UID of process"));
> -
> - /* Check executable bit for current user. */
> - if (apr_uid_compare(uid, file_info->user) == APR_SUCCESS)
> - *executable = (file_info->protection & APR_UEXECUTE);
> -
> - else if (apr_gid_compare(gid, file_info->group) == APR_SUCCESS)
> - *executable = (file_info->protection & APR_GEXECUTE);
> -
> - else
> - *executable = (file_info->protection & APR_WEXECUTE);
> -
> + svn_io_is_file_executable(executable, file_info->fname, pool);
> #else /* WIN32 || __OS2__ || !APR_HAS_USER */
> *executable = FALSE;
> #endif
> @@ -2599,12 +2576,7 @@ svn_io_is_file_executable(svn_boolean_t *executabl
> apr_pool_t *pool)
> {
> #if defined(APR_HAS_USER) && !defined(WIN32) &&!defined(__OS2__)
> - apr_finfo_t file_info;
> -
> - SVN_ERR(svn_io_stat(&file_info, path, APR_FINFO_PROT | APR_FINFO_OWNER,
> - pool));
> - SVN_ERR(svn_io__is_finfo_executable(executable, &file_info, pool));
> -
> + *executable = (access(path, X_OK) == 0);
> #else /* WIN32 || __OS2__ || !APR_HAS_USER */
> *executable = FALSE;
> #endif
> ]]]
>
> At the moment I've kept the apr_uid_current() call in
> svn_io__is_finfo_read_only(). My original plan was to extend
> svn_io__is_finfo_read_only to report if a file is readonly but owned by
> someone else (see [3]) but thinking about it again I think we should rather
> handle this in io_set_perms() - at the moment I'm running out of time but I
> will come back to this before final commit.
>
> Since this isn't my area of expertise, I'd appreciate if someone could
> review it before I commit.
>
Since there wasn't any replies to this and I think the code was working
fine in all my tests, I comitted as r1915214. Although I finally decided to
solve the spurious revert messages in a different way, see a separate
followup/committ e-mail.
Kind regards
Daniel
>
> Kind regards,
> Daniel Sahlberg
>
>
>
> [1] https://lists.apache.org/thread/p1ky889bxwy8okqly7h1lgckxfpldnxs
> [2] https://lists.apache.org/thread/3pr76fo7gzbqq397d9xvyd66gq5bwlmw
> [3] https://lists.apache.org/thread/sfkzqzgwr4wvvwfdcyhqgymhx6lmv24h
>
>
>
