Re: Changing the permission checks in libsvn_subr/io.c

[Daniel Sahlberg]

Den mån 5 feb. 2024 kl 07:38 skrev Branko Čibej :

> On 04.02.2024 00:31, Evgeny Kotkov via dev wrote:
>
> Daniel Sahlberg   
> writes:
>
>
> 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);
>
> There are a few problems, because this approach adds a I/O syscall (that
> checks access to a file by its name) into a fairly low-level function.
>
> Previously this function was analyzing the file information from the passed-in
> apr_finfo_t structure.  The new version, however, performs an I/O call, and
> that leads to a couple of problems:
>
> 1) Potential performance regression, not limited to just the revert.
>For example, all calls to open_pack_or_rev_file() in libsvn_fs_fs would
>now start to make additional access() calls, thus slowing down the
>repository operations.
>
> 2) This adds an opportunity for a desynchronization/race between the
>information in apr_finfo_t and the result of access(), because access()
>works by a filename.  For example, in a case where a file is recreated
>between the two calls, its result will correspond to a completely
>different file, compared to the one that produced the apr_finfo_t.
>
> Overall, I have a feeling that solving this specific edge-case might not be
> worth introducing these regressions (which look significant, at least to me).
>
>
> I agree. If it's worth solving, it should be done in a way that's specific
> to revert, not here.
>
> -- Brane
>

Thanks for the feedback and sorry for the late reply. I've been swamped
with other tasks lately and have not yet found the time to look at this but
it is for sure on my todo list.

Kind regards,
Daniel

Re: Changing the permission checks in libsvn_subr/io.c

[Branko Čibej]

On 04.02.2024 00:31, Evgeny Kotkov via dev wrote:

Daniel Sahlberg  writes:


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

There are a few problems, because this approach adds a I/O syscall (that
checks access to a file by its name) into a fairly low-level function.

Previously this function was analyzing the file information from the passed-in
apr_finfo_t structure.  The new version, however, performs an I/O call, and
that leads to a couple of problems:

1) Potential performance regression, not limited to just the revert.
For example, all calls to open_pack_or_rev_file() in libsvn_fs_fs would
now start to make additional access() calls, thus slowing down the
repository operations.

2) This adds an opportunity for a desynchronization/race between the
information in apr_finfo_t and the result of access(), because access()
works by a filename.  For example, in a case where a file is recreated
between the two calls, its result will correspond to a completely
different file, compared to the one that produced the apr_finfo_t.

Overall, I have a feeling that solving this specific edge-case might not be
worth introducing these regressions (which look significant, at least to me).


I agree. If it's worth solving, it should be done in a way that's 
specific to revert, not here.


-- Brane

Re: Changing the permission checks in libsvn_subr/io.c

[Evgeny Kotkov via dev]

Daniel Sahlberg  writes:

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

There are a few problems, because this approach adds a I/O syscall (that
checks access to a file by its name) into a fairly low-level function.

Previously this function was analyzing the file information from the passed-in
apr_finfo_t structure.  The new version, however, performs an I/O call, and
that leads to a couple of problems:

1) Potential performance regression, not limited to just the revert.
   For example, all calls to open_pack_or_rev_file() in libsvn_fs_fs would
   now start to make additional access() calls, thus slowing down the
   repository operations.

2) This adds an opportunity for a desynchronization/race between the
   information in apr_finfo_t and the result of access(), because access()
   works by a filename.  For example, in a case where a file is recreated
   between the two calls, its result will correspond to a completely
   different file, compared to the one that produced the apr_finfo_t.

Overall, I have a feeling that solving this specific edge-case might not be
worth introducing these regressions (which look significant, at least to me).


Thanks,
Evgeny Kotkov

Re: Changing the permission checks in libsvn_subr/io.c

[Vincent Lefevre]

On 2024-01-22 04:05:04 +0100, Branko Čibej wrote:
> On 13.01.2024 09:58, Daniel Sahlberg wrote:
> > 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.
> 
> I think you just made the code more complicated for no good reason. The
> situation you described and fixed applies only to working copies that are
> shared by different users.

Note that in my case, the working copy was not shared by different
users. It just happened that some files had a different owner (as
I had done a "cp" under root by mistake).

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: Changing the permission checks in libsvn_subr/io.c

[Daniel Sahlberg]

Den mån 22 jan. 2024 kl 04:05 skrev Branko Čibej :

> On 13.01.2024 09:58, Daniel Sahlberg wrote:
>
> 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.
>
>
> I think you just made the code more complicated for no good reason. The
> situation you described and fixed applies only to working copies that are
> shared by different users.
>
> You also have to ask yourself what happens when your user runs an "svn
> update" in the working copy where it's not the owner of the files and when
> your primary group doesn't have write access to the containing directory.
> In other words, I think you just fixed an edge case that has lots of other
> problems, not least that without using at least 'chgrp', the working copy
> will be either useless for you or quickly corrupted for others. I'm not
> even going to try to think about what happens to SQLite WAL files. Your
> exhaustive tests don't seem to cover these cases.
>
> In practice, such working copies tend to be used by prefixing all svn
> commands with 'sudo -u wc-owner' or starting with 'sudo -u wc-owner -s'.
> That's a solution that always works and doesn't require more code in svn
> that someone has to maintain and that only solves one very specific edge
> case.
>
> -- Brane
>
>
Are you referring to the changes in 1915214 or in 1915215?

r1915214 replaces a homegrown check for the current UID and GID, then
comparing with current file mask (total 23 loc) with calls to a library
function (total 4 loc). I presume there is a cost for some stat calls
within access(), so performance is probably a bit worse. (Maybe some of the
performance could be recovered in revert_wc_data, there is comment to use
svn_io_dirent2_t "[except] that we need the read only and execute bits
later", I presume for the now replaced checks). I would argue that the new
code is simpler and easier to maintain, but I'm of course open to other
opinions.

With r1915215, I can agree it makes the code more complicated. But it
solves another issue, the "spurious revert" messages reported here several
times (most recently by Vincent Lefevre). I'd be the first to admit that
the fix is still half-baked: The message is completely bonkers (and it
lacks a trailing \n), but at least the user is notified that something is
going on instead of getting the "reverted" message over and over again.

Kind regards,
Daniel

Re: Changing the permission checks in libsvn_subr/io.c

[Branko Čibej]

On 13.01.2024 09:58, Daniel Sahlberg wrote:
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.


I think you just made the code more complicated for no good reason. The 
situation you described and fixed applies only to working copies that 
are shared by different users.


You also have to ask yourself what happens when your user runs an "svn 
update" in the working copy where it's not the owner of the files and 
when your primary group doesn't have write access to the containing 
directory. In other words, I think you just fixed an edge case that has 
lots of other problems, not least that without using at least 'chgrp', 
the working copy will be either useless for you or quickly corrupted for 
others. I'm not even going to try to think about what happens to SQLite 
WAL files. Your exhaustive tests don't seem to cover these cases.


In practice, such working copies tend to be used by prefixing all svn 
commands with 'sudo -u wc-owner' or starting with 'sudo -u wc-owner -s'. 
That's a solution that always works and doesn't require more code in svn 
that someone has to maintain and that only solves one very specific edge 
case.


-- Brane

Re: Changing the permission checks in libsvn_subr/io.c

[Daniel Sahlberg]

Den ons 10 jan. 2024 kl 10:45 skrev Stefan Sperling :

> On Wed, Jan 10, 2024 at 09:44:51AM +0100, Johan Corveleyn wrote:
> > Interesting discussion. I agree it should at least be documented, and
> > perhaps be made a bit more clear from the output of 'revert' (but not
> > sure how far we can go without breaking compat). Changing the current
> > behavior is probably a more risky move, given the maturity of SVN and
> > backwards compatibility etc.
>
> Adding new notification types should not cause compatibility concerns.
> If adding new notifications breaks other software which reads command
> line client output than this other software has a bug: It should have
> been ignoring unknown lines of output in the first place.
>
> API users would likewise simply need to catch the new notification type
> in a switch-like statement and should also be ignoring unknown values.
>
> I would only see a compatibility problem if an existing notification
> about an important event no longer appears.
>
> In the past we have made significant changes to output from commands,
> such as when tree conflict detection was added in 1.6. That by itself
> has not resulted in any problems, as far as I know.
>

Thanks for the feedback (and sorry for mixing up the threads previously
saying there was no feedback here).

I've made a separate commit r1915215 adding code to catch the inconsistency
with the auth bits and report a separate notification.

Kind regards,
Daniel

Re: Changing the permission checks in libsvn_subr/io.c

[Daniel Sahlberg]

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(, , 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(, , pool);
> -
> -  if (apr_err)
> -return svn_error_wrap_apr(apr_err, _("Error 

Re: Changing the permission checks in libsvn_subr/io.c

[Vincent Lefevre]

On 2024-01-10 09:44:51 +0100, Johan Corveleyn wrote:
> Interesting discussion. I agree it should at least be documented, and
> perhaps be made a bit more clear from the output of 'revert' (but not
> sure how far we can go without breaking compat). Changing the current
> behavior is probably a more risky move, given the maturity of SVN and
> backwards compatibility etc.

Another solution would be to make the behavior configurable
(in .subversion/config).

[...]
> BTW, I know about a similar issue of 'spurious revert notifications',
> with mismatching timestamps (lastmod-time different from svn metadata,
> a condition that is normally fixed by 'svn cleanup'): if you "touch" a
> file in your WC so it has a different timestamp from the metadata, it
> will be notified when running 'svn revert' (and I believe the metadata
> is adjusted in this case). So that's another source of spurious revert
> notifications. I thought I had discussed this on one of our
> mailinglists, but I can't find it. The fact that revert fixes
> timestamps is mentioned in passing in issue SVN-4162.

But there are no properties related to the behavior concerning
timestamps (a "touch" may be done on purpose for timestamp-based
utilities like "make").

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: Changing the permission checks in libsvn_subr/io.c

[Stefan Sperling]

On Wed, Jan 10, 2024 at 09:44:51AM +0100, Johan Corveleyn wrote:
> Interesting discussion. I agree it should at least be documented, and
> perhaps be made a bit more clear from the output of 'revert' (but not
> sure how far we can go without breaking compat). Changing the current
> behavior is probably a more risky move, given the maturity of SVN and
> backwards compatibility etc.

Adding new notification types should not cause compatibility concerns.
If adding new notifications breaks other software which reads command
line client output than this other software has a bug: It should have
been ignoring unknown lines of output in the first place.

API users would likewise simply need to catch the new notification type
in a switch-like statement and should also be ignoring unknown values.

I would only see a compatibility problem if an existing notification
about an important event no longer appears.

In the past we have made significant changes to output from commands,
such as when tree conflict detection was added in 1.6. That by itself
has not resulted in any problems, as far as I know.

Re: Changing the permission checks in libsvn_subr/io.c

[Johan Corveleyn]

On Sun, Jan 7, 2024 at 2:46 AM Vincent Lefevre  wrote:
> On 2024-01-05 11:29:16 +0100, Daniel Sahlberg wrote:
> > Den fre 5 jan. 2024 kl 10:51 skrev Johan Corveleyn :
> >
> > > On Fri, Jan 5, 2024 at 8:46 AM Daniel Sahlberg
> > >  wrote:
> > > ...
> > > > 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 ...]
> > >
> > > Should it? Intuitively I'd say: since the file doesn't have
> > > svn:needs-lock Subversion shouldn't be looking at R or RW. Why should
> > > we make a file RW? Can't the user make a file readonly just locally,
> > > and expect Subversion not to care?
> > >
> > > Or is "making a file readonly" a committable local change? Will it
> > > show up on 'svn st' and can it be committed as some change that can be
> > > transferred to another working copy?
> > >
> > > I understand that svn:needs-lock adds extra handling of the readonly
> > > status of files, but without that property?
> >
> > All good questions, and I probably agree with you: if svn:needs-lock isn't
> > set then Subversion could just ignore the R/RW status.
>
> I also agree. I never use svn:needs-lock, and I want to be able to
> set some files in my working copy to read-only, in order to make sure
> that I won't modify them by mistake.
>
> > But any change here would change previous behaviour so it would need
> > a solid consensus.
>
> In any case, the current behavior of Subversion is inconsistent.
> "svn revert" is not documented as the command to "fix" the permissions.
>
> > If the check is removed for files that doesn't svn:needs-lock, then we
> > might have to add code to restore RW status if svn:needs-lock is removed.
> >
> > Making a file readonly is currently not a committable change, didn't check
> > 'svn st' but it will be reverted by 'svn revert' and it will not be
> > transferred to another WC. It can only be committed indirectly via
> > svn:needs-lock.
> >
> > Any discussion regarding svn:needs-lock probably also have to consider
> > svn:executable, since it is handled similarly (except on WIN32 and OS2,
> > where the concept of executable doesn't exists).
> >
> > I havn't completely made up my mind, but I think I favour keeping the
> > current behaviour: R/RW status in indicated by the svn:needs-lock property
> > and you shouldn't change R/RW manually within a WC.
>
> Then this should be documented.
>
> But "svn st" should detect and report incorrect permissions, and
> "svn up" should fix them, just like what happens when a file has
> been removed with just "rm" instead of "svn delete".

Interesting discussion. I agree it should at least be documented, and
perhaps be made a bit more clear from the output of 'revert' (but not
sure how far we can go without breaking compat). Changing the current
behavior is probably a more risky move, given the maturity of SVN and
backwards compatibility etc.

BTW, I did some more digging because the problem looked familiar, and
apparently there was a discussion on users@ in 2016 [1] in which I
participated :). It was reported with a Windows client. I ended up
filing an issue which contains some interesting background information
by Bert [2]. A quote that stood out to me just now when I was
rereading it:

"For WC-NG we explicitly made 'svn revert' always revert your working
copy match how it would be after a clean checkout. (I think philipm
worked on this at the time). And he did quite a bit more work to
ensure that we always provide a notification when we change the
working copy. (Older versions could change your working copy, but not
notify you about this)
...
I think it might have been better to use a specific notify for 'fixing
read-only-ness' during revert, but I'm not sure if this is really
worth all the backwards compatibility breakage to apply this three
versions after this behavior was introduced."

BTW, I know about a similar issue of 'spurious revert notifications',
with mismatching timestamps (lastmod-time different from svn metadata,
a condition that is normally fixed by 'svn cleanup'): if you "touch" a
file in your WC so it has a different timestamp from the metadata, it
will be notified when running 'svn revert' (and I believe the metadata
is adjusted in this case). So that's another source of spurious revert
notifications. I thought I had discussed this on one of our
mailinglists, but I can't find it. The fact that revert fixes
timestamps is mentioned in passing in issue SVN-4162.

Perhaps both types of "adjustments" by revert should get a specific
notification (if any -- for the timestamp fixup case I can live with
simply not notifying, it's an adjustment in metadata, not on-disk file
after all).

[1] https://svn.haxx.se/users/archive-2016-03/0004.shtml
[2] https://issues.apache.org/jira/browse/SVN-4637 (Revert affects
unchanged files with changed permissions)
[3] https://issues.apache.org/jira/browse/SVN-4162 (make svn status
fix timestamp mismatches in meta-data)

-- 
Johan

Re: Changing the permission checks in libsvn_subr/io.c

[Vincent Lefevre]

On 2024-01-05 11:29:16 +0100, Daniel Sahlberg wrote:
> Den fre 5 jan. 2024 kl 10:51 skrev Johan Corveleyn :
> 
> > On Fri, Jan 5, 2024 at 8:46 AM Daniel Sahlberg
> >  wrote:
> > ...
> > > 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 ...]
> >
> > Should it? Intuitively I'd say: since the file doesn't have
> > svn:needs-lock Subversion shouldn't be looking at R or RW. Why should
> > we make a file RW? Can't the user make a file readonly just locally,
> > and expect Subversion not to care?
> >
> > Or is "making a file readonly" a committable local change? Will it
> > show up on 'svn st' and can it be committed as some change that can be
> > transferred to another working copy?
> >
> > I understand that svn:needs-lock adds extra handling of the readonly
> > status of files, but without that property?
> 
> All good questions, and I probably agree with you: if svn:needs-lock isn't
> set then Subversion could just ignore the R/RW status.

I also agree. I never use svn:needs-lock, and I want to be able to
set some files in my working copy to read-only, in order to make sure
that I won't modify them by mistake.

> But any change here would change previous behaviour so it would need
> a solid consensus.

In any case, the current behavior of Subversion is inconsistent.
"svn revert" is not documented as the command to "fix" the permissions.

> If the check is removed for files that doesn't svn:needs-lock, then we
> might have to add code to restore RW status if svn:needs-lock is removed.
> 
> Making a file readonly is currently not a committable change, didn't check
> 'svn st' but it will be reverted by 'svn revert' and it will not be
> transferred to another WC. It can only be committed indirectly via
> svn:needs-lock.
> 
> Any discussion regarding svn:needs-lock probably also have to consider
> svn:executable, since it is handled similarly (except on WIN32 and OS2,
> where the concept of executable doesn't exists).
> 
> I havn't completely made up my mind, but I think I favour keeping the
> current behaviour: R/RW status in indicated by the svn:needs-lock property
> and you shouldn't change R/RW manually within a WC.

Then this should be documented.

But "svn st" should detect and report incorrect permissions, and
"svn up" should fix them, just like what happens when a file has
been removed with just "rm" instead of "svn delete".

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: Changing the permission checks in libsvn_subr/io.c

[Daniel Sahlberg]

Den fre 5 jan. 2024 kl 10:51 skrev Johan Corveleyn :

> On Fri, Jan 5, 2024 at 8:46 AM Daniel Sahlberg
>  wrote:
> ...
> > 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 ...]
>
> Should it? Intuitively I'd say: since the file doesn't have
> svn:needs-lock Subversion shouldn't be looking at R or RW. Why should
> we make a file RW? Can't the user make a file readonly just locally,
> and expect Subversion not to care?
>
> Or is "making a file readonly" a committable local change? Will it
> show up on 'svn st' and can it be committed as some change that can be
> transferred to another working copy?
>
> I understand that svn:needs-lock adds extra handling of the readonly
> status of files, but without that property?
>

All good questions, and I probably agree with you: if svn:needs-lock isn't
set then Subversion could just ignore the R/RW status. But any change here
would change previous behaviour so it would need a solid consensus.

If the check is removed for files that doesn't svn:needs-lock, then we
might have to add code to restore RW status if svn:needs-lock is removed.

Making a file readonly is currently not a committable change, didn't check
'svn st' but it will be reverted by 'svn revert' and it will not be
transferred to another WC. It can only be committed indirectly via
svn:needs-lock.

Any discussion regarding svn:needs-lock probably also have to consider
svn:executable, since it is handled similarly (except on WIN32 and OS2,
where the concept of executable doesn't exists).

I havn't completely made up my mind, but I think I favour keeping the
current behaviour: R/RW status in indicated by the svn:needs-lock property
and you shouldn't change R/RW manually within a WC.

/Daniel

Re: Changing the permission checks in libsvn_subr/io.c

[Johan Corveleyn]

On Fri, Jan 5, 2024 at 8:46 AM Daniel Sahlberg
 wrote:
...
> 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 ...]

Should it? Intuitively I'd say: since the file doesn't have
svn:needs-lock Subversion shouldn't be looking at R or RW. Why should
we make a file RW? Can't the user make a file readonly just locally,
and expect Subversion not to care?

Or is "making a file readonly" a committable local change? Will it
show up on 'svn st' and can it be committed as some change that can be
transferred to another working copy?

I understand that svn:needs-lock adds extra handling of the readonly
status of files, but without that property?

-- 
Johan

Changing the permission checks in libsvn_subr/io.c

[Daniel Sahlberg]

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(, , 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(, , 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 =