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