You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ds...@apache.org on 2024/01/13 09:16:26 UTC
svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Author: dsahlberg
Date: Sat Jan 13 09:16:26 2024
New Revision: 1915215
URL: http://svn.apache.org/viewvc?rev=1915215&view=rev
Log:
Manage spurious Reverted message caused by non-W access to files
owned by another user. Part of Issue #4622.
The revert notification comes from the code trying to add W permissions
but since there is already W (for another user) the code doesn't change
anything and the notification will come back next time as well.
Changing to add a separate notification type "you don't have W access
and we can't do anything about it".
The text should be tweaked further.
Discussed on dev@: https://lists.apache.org/thread/p1ky889bxwy8okqly7h1lgckxfpldnxs
* subversion/include/svn_wc.h
(svn_wc_notify_action_t): Add a new notification type
* subversion/libsvn_wc/revert.c
(revert_wc_data): Add new parameter to indicate the need for notification
of "no access" and use that when a file is readonly but
(some other user) already has W.
(revert_restore): Handle the "no access" case with the new notification type.
* subversion/svn/notify.c
(notify_body): Handle the new notification type
* subversion/svnbench/notify.c
(notify): Handle the new notification type
Modified:
subversion/trunk/subversion/include/svn_wc.h
subversion/trunk/subversion/libsvn_wc/revert.c
subversion/trunk/subversion/svn/notify.c
subversion/trunk/subversion/svnbench/notify.c
Modified: subversion/trunk/subversion/include/svn_wc.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_wc.h?rev=1915215&r1=1915214&r2=1915215&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_wc.h (original)
+++ subversion/trunk/subversion/include/svn_wc.h Sat Jan 13 09:16:26 2024
@@ -993,6 +993,7 @@ typedef enum svn_wc_notify_action_t
svn_wc_notify_restore,
/** Reverting a modified path. */
+ /* See also svn_wc_notify_revert_noaccess */
svn_wc_notify_revert,
/** A revert operation has failed. */
@@ -1325,6 +1326,12 @@ typedef enum svn_wc_notify_action_t
* @since New in 1.15. */
svn_wc_notify_warning,
+ /** A file is readonly for the user but isn't svn:needs-lock.
+ * So we want to restore RW, but fail since the file has W bits,
+ * just not for the current user.
+ * @since New in 1.15. */
+ svn_wc_notify_revert_noaccess,
+
} svn_wc_notify_action_t;
Modified: subversion/trunk/subversion/libsvn_wc/revert.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/revert.c?rev=1915215&r1=1915214&r2=1915215&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/revert.c (original)
+++ subversion/trunk/subversion/libsvn_wc/revert.c Sat Jan 13 09:16:26 2024
@@ -263,6 +263,7 @@ revert_restore_handle_copied_dirs(svn_bo
static svn_error_t *
revert_wc_data(svn_boolean_t *run_wq,
svn_boolean_t *notify_required,
+ svn_boolean_t *notify_noaccess,
svn_wc__db_t *db,
const char *local_abspath,
svn_wc__db_status_t status,
@@ -309,6 +310,7 @@ revert_restore(svn_boolean_t *run_wq,
svn_wc__db_status_t status;
svn_node_kind_t kind;
svn_boolean_t notify_required;
+ svn_boolean_t notify_noaccess;
const apr_array_header_t *conflict_files;
svn_filesize_t recorded_size;
apr_time_t recorded_time;
@@ -398,7 +400,7 @@ revert_restore(svn_boolean_t *run_wq,
if (!metadata_only)
{
SVN_ERR(revert_wc_data(run_wq,
- ¬ify_required,
+ ¬ify_required, ¬ify_noaccess,
db, local_abspath, status, kind,
reverted_kind, recorded_size, recorded_time,
copied_here, use_commit_times,
@@ -419,12 +421,19 @@ revert_restore(svn_boolean_t *run_wq,
}
}
- if (notify_func && notify_required)
- notify_func(notify_baton,
- svn_wc_create_notify(local_abspath, svn_wc_notify_revert,
- scratch_pool),
- scratch_pool);
-
+ if (notify_func)
+ {
+ if (notify_required)
+ notify_func(notify_baton,
+ svn_wc_create_notify(local_abspath, svn_wc_notify_revert,
+ scratch_pool),
+ scratch_pool);
+ else if (notify_noaccess)
+ notify_func(notify_baton,
+ svn_wc_create_notify(local_abspath, svn_wc_notify_revert_noaccess,
+ scratch_pool),
+ scratch_pool);
+ }
if (depth == svn_depth_infinity && kind == svn_node_dir)
{
apr_pool_t *iterpool = svn_pool_create(scratch_pool);
@@ -482,6 +491,7 @@ revert_restore(svn_boolean_t *run_wq,
static svn_error_t *
revert_wc_data(svn_boolean_t *run_wq,
svn_boolean_t *notify_required,
+ svn_boolean_t *notify_noaccess,
svn_wc__db_t *db,
const char *local_abspath,
svn_wc__db_status_t status,
@@ -661,11 +671,23 @@ revert_wc_data(svn_boolean_t *run_wq,
}
else if (!needs_lock_prop && read_only)
{
- SVN_ERR(svn_io_set_file_read_write(local_abspath,
- FALSE,
- scratch_pool));
- *notify_required = TRUE;
- }
+ /* If there is already W on the file, it is owned by
+ * some other user. Then svn_io_set_file_read_write
+ * will return without making any changes and the
+ * user will get a spurious "Reverted" message.
+ * Only checking for user's W since that is the only
+ * one set by svn_io_set_file_read_write()
+ * Issue #4622 */
+ if (finfo.protection | APR_UWRITE)
+ *notify_noaccess = TRUE;
+ else
+ {
+ SVN_ERR(svn_io_set_file_read_write(local_abspath,
+ FALSE,
+ scratch_pool));
+ *notify_required = TRUE;
+ }
+ }
}
#if !defined(WIN32) && !defined(__OS2__)
Modified: subversion/trunk/subversion/svn/notify.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/notify.c?rev=1915215&r1=1915214&r2=1915215&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/notify.c (original)
+++ subversion/trunk/subversion/svn/notify.c Sat Jan 13 09:16:26 2024
@@ -450,6 +450,11 @@ notify_body(struct notify_baton *nb,
path_local));
break;
+ case svn_wc_notify_revert_noaccess:
+ SVN_ERR(svn_cmdline_printf(pool, _("User doesn't have WRITE permissions to file '%s' and the file isn't svn:needslock. But the file is already writeable. Probably owned by another user."),
+ path_local));
+ break;
+
case svn_wc_notify_failed_revert:
SVN_ERR(svn_cmdline_printf(pool, _("Failed to revert '%s' -- "
"try updating instead.\n"),
Modified: subversion/trunk/subversion/svnbench/notify.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnbench/notify.c?rev=1915215&r1=1915214&r2=1915215&view=diff
==============================================================================
--- subversion/trunk/subversion/svnbench/notify.c (original)
+++ subversion/trunk/subversion/svnbench/notify.c Sat Jan 13 09:16:26 2024
@@ -241,6 +241,12 @@ notify(void *baton, const svn_wc_notify_
goto print_error;
break;
+ case svn_wc_notify_revert_noaccess:
+ if ((err = svn_cmdline_printf(pool, _("User doesn't have WRITE permissions to file '%s' and the file isn't svn:needslock. But the file is already writeable. Probably owned by another user."),
+ path_local)))
+ goto print_error;
+ break;
+
case svn_wc_notify_failed_revert:
if (( err = svn_cmdline_printf(pool, _("Failed to revert '%s' -- "
"try updating instead.\n"),
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Daniel Sahlberg <da...@gmail.com>.
Den lör 13 jan. 2024 kl 10:16 skrev <ds...@apache.org>:
> Author: dsahlberg
> Date: Sat Jan 13 09:16:26 2024
> New Revision: 1915215
>
> URL: http://svn.apache.org/viewvc?rev=1915215&view=rev
> Log:
> Manage spurious Reverted message caused by non-W access to files
> owned by another user. Part of Issue #4622.
>
> The revert notification comes from the code trying to add W permissions
> but since there is already W (for another user) the code doesn't change
> anything and the notification will come back next time as well.
>
> Changing to add a separate notification type "you don't have W access
> and we can't do anything about it".
>
> The text should be tweaked further.
>
...
Modified: subversion/trunk/subversion/svn/notify.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/notify.c?rev=1915215&r1=1915214&r2=1915215&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/notify.c (original)
+++ subversion/trunk/subversion/svn/notify.c Sat Jan 13 09:16:26 2024
@@ -450,6 +450,11 @@ notify_body(struct notify_baton *nb,
path_local));
break;
+ case svn_wc_notify_revert_noaccess:
+ SVN_ERR(svn_cmdline_printf(pool, _("User doesn't have WRITE
permissions to file '%s' and the file isn't svn:needslock. But the file is
already writeable. Probably owned by another user."),
+ path_local));
+ break;
+
case svn_wc_notify_failed_revert:
SVN_ERR(svn_cmdline_printf(pool, _("Failed to revert '%s' -- "
"try updating instead.\n"),
Modified: subversion/trunk/subversion/svnbench/notify.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/svnbench/notify.c?rev=1915215&r1=1915214&r2=1915215&view=diff
==============================================================================
--- subversion/trunk/subversion/svnbench/notify.c (original)
+++ subversion/trunk/subversion/svnbench/notify.c Sat Jan 13 09:16:26 2024
@@ -241,6 +241,12 @@ notify(void *baton, const svn_wc_notify_
goto print_error;
break;
+ case svn_wc_notify_revert_noaccess:
+ if ((err = svn_cmdline_printf(pool, _("User doesn't have WRITE
permissions to file '%s' and the file isn't svn:needslock. But the file is
already writeable. Probably owned by another user."),
+ path_local)))
+ goto print_error;
+ break;
+
case svn_wc_notify_failed_revert:
if (( err = svn_cmdline_printf(pool, _("Failed to revert '%s' -- "
"try updating instead.\n"),
I would REALLY like someone to suggest a better wording for these messages.
I couldn't come up with something simple that both explains what happend
and suggests a reason.
Kind regards,
Daniel
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Daniel Sahlberg <da...@gmail.com>.
Den mån 29 jan. 2024 kl 13:26 skrev Jun Omae <ju...@gmail.com>:
> On 2024/01/13 18:16, dsahlberg@apache.org wrote:
> > Author: dsahlberg
> > Date: Sat Jan 13 09:16:26 2024
> > New Revision: 1915215
> >
> > URL: http://svn.apache.org/viewvc?rev=1915215&view=rev
> > Log:
> > Manage spurious Reverted message caused by non-W access to files
> > owned by another user. Part of Issue #4622.
> >
> > The revert notification comes from the code trying to add W permissions
> > but since there is already W (for another user) the code doesn't change
> > anything and the notification will come back next time as well.
> >
> > Changing to add a separate notification type "you don't have W access
> > and we can't do anything about it".
> >
> > The text should be tweaked further.
> >
> > Discussed on dev@:
> https://lists.apache.org/thread/p1ky889bxwy8okqly7h1lgckxfpldnxs
>
> After r1915215, make check and check-apache-javahl failed.
>
> [[[
> Summary of test results:
> 2558 tests PASSED
> 168 tests SKIPPED
> 81 tests XFAILED (17 WORK-IN-PROGRESS)
> 27 tests FAILED
> ]]]
>
That was a stupid mistake on my part. I obviously only checked part of the
testsuite. After r1915466, the testsuite passes again.
>
> [[[
> There was 1 error:
> 1)
> testDiff(org.apache.subversion.javahl.BasicTests)org.apache.subversion.javahl.ClientException:
> The operation was interrupted
> svn: Operation cancelled
> svn: Wrapped Java Exception
>
> at org.apache.subversion.javahl.SVNClient.revert(Native Method)
> at org.apache.subversion.javahl.SVNClient.revert(SVNClient.java:253)
> at
> org.apache.subversion.javahl.BasicTests.testDiff(BasicTests.java:3419)
> at
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at org.apache.subversion.javahl.RunTests.main(RunTests.java:119)
> Caused by: java.lang.ArrayIndexOutOfBoundsException: Index 90 out of
> bounds for length 80
> ... 19 more
>
> FAILURES!!!
> Tests run: 148, Failures: 0, Errors: 1
>
> make: *** [Makefile:534: check-apache-javahl] Error 1
> ]]]
>
The java tests also pass for me now.
Kind regards,
Daniel
>
> --
> Jun Omae <ju...@gmail.com> (大前 潤)
>
>
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Jun Omae <ju...@gmail.com>.
On 2024/01/13 18:16, dsahlberg@apache.org wrote:
> Author: dsahlberg
> Date: Sat Jan 13 09:16:26 2024
> New Revision: 1915215
>
> URL: http://svn.apache.org/viewvc?rev=1915215&view=rev
> Log:
> Manage spurious Reverted message caused by non-W access to files
> owned by another user. Part of Issue #4622.
>
> The revert notification comes from the code trying to add W permissions
> but since there is already W (for another user) the code doesn't change
> anything and the notification will come back next time as well.
>
> Changing to add a separate notification type "you don't have W access
> and we can't do anything about it".
>
> The text should be tweaked further.
>
> Discussed on dev@: https://lists.apache.org/thread/p1ky889bxwy8okqly7h1lgckxfpldnxs
After r1915215, make check and check-apache-javahl failed.
[[[
Summary of test results:
2558 tests PASSED
168 tests SKIPPED
81 tests XFAILED (17 WORK-IN-PROGRESS)
27 tests FAILED
]]]
[[[
There was 1 error:
1) testDiff(org.apache.subversion.javahl.BasicTests)org.apache.subversion.javahl.ClientException: The operation was interrupted
svn: Operation cancelled
svn: Wrapped Java Exception
at org.apache.subversion.javahl.SVNClient.revert(Native Method)
at org.apache.subversion.javahl.SVNClient.revert(SVNClient.java:253)
at org.apache.subversion.javahl.BasicTests.testDiff(BasicTests.java:3419)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at org.apache.subversion.javahl.RunTests.main(RunTests.java:119)
Caused by: java.lang.ArrayIndexOutOfBoundsException: Index 90 out of bounds for length 80
... 19 more
FAILURES!!!
Tests run: 148, Failures: 0, Errors: 1
make: *** [Makefile:534: check-apache-javahl] Error 1
]]]
--
Jun Omae <ju...@gmail.com> (大前 潤)
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Evgeny Kotkov via dev <de...@subversion.apache.org>.
Daniel Sahlberg <da...@gmail.com> writes:
>> If we decide to go this route, I would say it might be better to move the
>> whole topic to 1.16. What are your thoughts on how this fits into the 1.15
>> plan?
>
> I have had another look at it and it is probably better to revert this code
> completely and rework the whole authorization validation. No time to do this
> before 1.15 though so it should be left for 1.16. I have reverted this code.
Thanks!
I understand that reverting isn't the preferred outcome, but it's the right
call in my opinion, and we can have this topic on our radars for 1.16.
Regards,
Evgeny Kotkov
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Daniel Sahlberg <da...@gmail.com>.
Den mån 22 dec. 2025 kl 17:51 skrev Evgeny Kotkov <
evgeny.kotkov@axiorema.com>:
> Daniel Sahlberg <da...@gmail.com> writes:
>
> > Do you agree that it is enough to initialize the variable to FALSE or
> > would you prefer that I rework the whole feature in some way?
>
> Hi Daniel,
>
> After taking a closer look at the whole change, I'm not sure I agree with
> the overall direction of the fix.
>
> As far as I can tell, the issue stems from inconsistencies in the Unix
> implementations of functions like svn_io__is_finfo_read_only() and
> svn_io_set_file_read_write(), such as:
>
> • A file without world write permissions is seen as read-only for root.
> • After calling svn_io_set_file_read_write(TRUE) for such file without an
> error, it is still reported as read-only by svn_io__is_finfo_read_only().
>
> So incorrect notifications are probably just a symptom of a deeper set of
> problems. And these problems may also affect other areas as well, e.g.
> when used by libsvn_fs_fs/rev_file.c:auto_make_writable(). In other words,
> I don't think this is a revert-specific issue; it just happens to be more
> visible there because the notifications are user-facing.
>
> The current change results in a rather verbose notification that may also
> look confusing to the users:
>
> > svn revert -R .
> User doesn't have WRITE permissions to file 'etc/apache2/mods-available/
> dnssd.load' and the file isn't svn:needslock. But the file is already
> writeable. Probably owned by another user.
> User doesn't have WRITE permissions to file 'etc/apache2/mods-available/
> dnssd.conf' and the file isn't svn:needslock. But the file is already
> writeable. Probably owned by another user.
> User doesn't have WRITE permissions to file 'etc/apache2/conf-available/
> javascript-common.conf' and the file isn't svn:needslock. But the file
> is already writeable. Probably owned by another user.
> …
>
> Perhaps, instead of trying to communicate this via the notifications, we
> could
> look into fixing the root cause so that the existing notification logic
> would
> automatically work as intended?
>
> One potential step forward would be to try updating the functions to
> correctly
> work for the `root` user, as mentioned in the original issue report.
> E.g., we
> could create a matrix of permission variations and the expected behavior
> for
> each, and then see if the implementation can be adjusted to match.
>
> If we decide to go this route, I would say it might be better to move the
> whole topic to 1.16. What are your thoughts on how this fits into the 1.15
> plan?
>
I have had another look at it and it is probably better to revert this code
completely and rework the whole authorization validation. No time to do
this before 1.15 though so it should be left for 1.16. I have reverted this
code.
Cheers,
Daniel
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Evgeny Kotkov via dev <de...@subversion.apache.org>.
Daniel Sahlberg <da...@gmail.com> writes:
> Do you agree that it is enough to initialize the variable to FALSE or
> would you prefer that I rework the whole feature in some way?
Hi Daniel,
After taking a closer look at the whole change, I'm not sure I agree with
the overall direction of the fix.
As far as I can tell, the issue stems from inconsistencies in the Unix
implementations of functions like svn_io__is_finfo_read_only() and
svn_io_set_file_read_write(), such as:
• A file without world write permissions is seen as read-only for root.
• After calling svn_io_set_file_read_write(TRUE) for such file without an
error, it is still reported as read-only by svn_io__is_finfo_read_only().
So incorrect notifications are probably just a symptom of a deeper set of
problems. And these problems may also affect other areas as well, e.g.
when used by libsvn_fs_fs/rev_file.c:auto_make_writable(). In other words,
I don't think this is a revert-specific issue; it just happens to be more
visible there because the notifications are user-facing.
The current change results in a rather verbose notification that may also
look confusing to the users:
> svn revert -R .
User doesn't have WRITE permissions to file 'etc/apache2/mods-available/
dnssd.load' and the file isn't svn:needslock. But the file is already
writeable. Probably owned by another user.
User doesn't have WRITE permissions to file 'etc/apache2/mods-available/
dnssd.conf' and the file isn't svn:needslock. But the file is already
writeable. Probably owned by another user.
User doesn't have WRITE permissions to file 'etc/apache2/conf-available/
javascript-common.conf' and the file isn't svn:needslock. But the file
is already writeable. Probably owned by another user.
…
Perhaps, instead of trying to communicate this via the notifications, we could
look into fixing the root cause so that the existing notification logic would
automatically work as intended?
One potential step forward would be to try updating the functions to correctly
work for the `root` user, as mentioned in the original issue report. E.g., we
could create a matrix of permission variations and the expected behavior for
each, and then see if the implementation can be adjusted to match.
If we decide to go this route, I would say it might be better to move the
whole topic to 1.16. What are your thoughts on how this fits into the 1.15
plan?
Thanks,
Evgeny Kotkov
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Daniel Sahlberg <da...@gmail.com>.
Evgeny,
Den mån 15 dec. 2025 kl 22:40 skrev Daniel Sahlberg <
daniel.l.sahlberg@gmail.com>:
...
> [[[
> $ svn diff libsvn_wc/revert.c
> Index: libsvn_wc/revert.c
> ===================================================================
> --- libsvn_wc/revert.c (revision 1930472)
> +++ libsvn_wc/revert.c (working copy)
> @@ -310,7 +310,7 @@
> svn_wc__db_status_t status;
> svn_node_kind_t kind;
> svn_boolean_t notify_required;
> - svn_boolean_t notify_noaccess;
> + svn_boolean_t notify_noaccess = FALSE;
> const apr_array_header_t *conflict_files;
> svn_filesize_t recorded_size;
> apr_time_t recorded_time;
> ]]]
>
Do you agree that it is enough to initialize the variable to FALSE or would
you prefer that I rework the whole feature in some way?
Cheers,
Daniel
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Daniel Sahlberg <da...@gmail.com>.
Den mån 15 dec. 2025 kl 15:32 skrev Evgeny Kotkov via dev <
dev@subversion.apache.org>:
> Daniel Sahlberg <ds...@apache.org> writes:
>
> > svn_boolean_t notify_required;
> > + svn_boolean_t notify_noaccess;
> …
> > if (!metadata_only)
> > {
> > SVN_ERR(revert_wc_data(run_wq,
> > - ¬ify_required,
> > + ¬ify_required, ¬ify_noaccess,
> > db, local_abspath, status, kind,
> > reverted_kind, recorded_size,
> recorded_time,
> > copied_here, use_commit_times,
> …
> > + if (notify_func)
> > + {
> > + if (notify_required)
> > + notify_func(notify_baton,
> > + svn_wc_create_notify(local_abspath,
> svn_wc_notify_revert,
> > + scratch_pool),
> > + scratch_pool);
> > + else if (notify_noaccess)
> > + notify_func(notify_baton,
> > + svn_wc_create_notify(local_abspath,
> svn_wc_notify_revert_noaccess,
> > + scratch_pool),
> > + scratch_pool);
> > + }
>
> It looks like this may have introduced an undefined behavior for reverts
> with
> metadata_only = TRUE.
>
> If metadata_only is TRUE, the block calling revert_wc_data is skipped,
> meaning
> notify_noaccess is never assigned a value. However, the variable might
> still
> be read later in the `else if (notify_noaccess)` check.
>
You are right of course, good catch.
How about:
[[[
$ svn diff libsvn_wc/revert.c
Index: libsvn_wc/revert.c
===================================================================
--- libsvn_wc/revert.c (revision 1930472)
+++ libsvn_wc/revert.c (working copy)
@@ -310,7 +310,7 @@
svn_wc__db_status_t status;
svn_node_kind_t kind;
svn_boolean_t notify_required;
- svn_boolean_t notify_noaccess;
+ svn_boolean_t notify_noaccess = FALSE;
const apr_array_header_t *conflict_files;
svn_filesize_t recorded_size;
apr_time_t recorded_time;
]]]
Cheers,
Daniel
>
>
Re: svn commit: r1915215 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revert.c svn/notify.c svnbench/notify.c
Posted by Evgeny Kotkov via dev <de...@subversion.apache.org>.
Daniel Sahlberg <ds...@apache.org> writes:
> svn_boolean_t notify_required;
> + svn_boolean_t notify_noaccess;
…
> if (!metadata_only)
> {
> SVN_ERR(revert_wc_data(run_wq,
> - ¬ify_required,
> + ¬ify_required, ¬ify_noaccess,
> db, local_abspath, status, kind,
> reverted_kind, recorded_size, recorded_time,
> copied_here, use_commit_times,
…
> + if (notify_func)
> + {
> + if (notify_required)
> + notify_func(notify_baton,
> + svn_wc_create_notify(local_abspath, svn_wc_notify_revert,
> + scratch_pool),
> + scratch_pool);
> + else if (notify_noaccess)
> + notify_func(notify_baton,
> + svn_wc_create_notify(local_abspath, svn_wc_notify_revert_noaccess,
> + scratch_pool),
> + scratch_pool);
> + }
It looks like this may have introduced an undefined behavior for reverts with
metadata_only = TRUE.
If metadata_only is TRUE, the block calling revert_wc_data is skipped, meaning
notify_noaccess is never assigned a value. However, the variable might still
be read later in the `else if (notify_noaccess)` check.
Thanks,
Evgeny Kotkov