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.
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
