Re: [PATCH] Make sure resolving a conflict with `svn_wc_conflict_choose_merged' can't break the workqueue by referencing a non-existing file
Thank you for your reply.
> Should we copy file permission bits as well as content?
> For example, with the svn_io_copy_perms() function?
As far as I can see the subsequent OP_FILE_INSTALL will not copy
permissions,
so copying permissions for the temporary file is not going to
change the behavior.
> Or perhaps this entire copy operation could be done with
svn_io_copy_file(),
> and with the copy_perms parameter set to TRUE? If this is possible then
the
> code would be a lot simpler.
The svn_io_copy_file() function makes another temp file and
performs a svn_io_file_rename2() on for atomicity. For this case, I think
that it would be better to avoid creating additional
temporary files.
I've updated the log message -- firstly I've missed changes in tests.
Log message:
[[[
Make sure resolving a conflict with `svn_wc_conflict_choose_merged' can't
break the workqueue by referencing a non-existing file
* subversion/libsvn_wc/conflicts.c
(build_text_conflict_resolve_items): Create private copy of MERGED_FILE
contents in case of `svn_wc_conflict_choose_merged' is specified.
Doing that to avoid referencing a potentially absent file in FILE_INSTALL
workqueue record.
* subversion/tests/libsvn_client/conflicts-test.c
(test_resolve_choose_merged_non_existing_path): New test.
(test_funcs): Add new test to list.
Patch by: sergey.raevskiy{_AT_}visualsvn.com
]]]
On Thu, Apr 21, 2022 at 10:04 PM Stefan Sperling wrote:
> On Thu, Apr 21, 2022 at 08:25:32PM +0300, Sergey Raevskiy wrote:
> > I've noticed that in some cases a working copy can become inoperable
> > after merge text conflict resolution with `svn_wc_conflict_choose_merged'
> > option.
> >
> > The problem can occur in the following scenario:
> >
> > 1. There is a text conflict, that is resolved with
> > `svn_wc_conflict_choose_merged' option and specifying some
> > path as MERGED_FILE. According to the API it can be any file,
> > let's say "D:\merged.txt".
> >
> > 2. Previous step adds a FILE_INSTALL item into workqueue with a
> > reference to "D:\merged.txt" and then runs the workqueue.
> >
> > 3. An error happens when running the workqueue.
> > For example a working file cannot be overwritten because
> > of 'Access denied' error.
> >
> > 4. The "D:\merged.txt" file gets deleted for some reason.
> >
> > 5. At this point the working copy is inoperable and cannot be
> > fixed by 'svn cleanup', because workqueue contains absolute
> > path to non-existing file ("D:\merged.txt").
> >
> > I've attached a patch with fix for this problem. The patch contains a
> > simplified regression test that reproduces problem by returning
> > non-existing path from resolver callback.
> >
> > Log message:
> > [[[
> > Make sure resolving a conflict with `svn_wc_conflict_choose_merged' can't
> > break the workqueue by referencing a non-existing file
> >
> > * subversion/libsvn_wc/conflicts.c
> > (build_text_conflict_resolve_items): Create private copy of MERGED_FILE
> >contents in case of `svn_wc_conflict_choose_merged' is specified.
> >Doing that to avoid referencing a potentially absent file in
> FILE_INSTALL
> >workqueue record.
> >
> > Patch by: sergey.raevskiy{_AT_}visualsvn.com
> > ]]]
>
> Thanks, Sergey! Your fix makes sense to me. One question below:
>
> > Index: subversion/libsvn_wc/conflicts.c
> > ===
> > --- subversion/libsvn_wc/conflicts.c (revision 1900114)
> > +++ subversion/libsvn_wc/conflicts.c (working copy)
> > @@ -1706,9 +1706,49 @@ build_text_conflict_resolve_items(svn_skel_t **wor
> > good to use". */
> >case svn_wc_conflict_choose_merged:
> > {
> > - install_from_abspath = merged_file
> > - ? merged_file
> > - : local_abspath;
> > + if (merged_file)
> > +{
> > + /* Create private copy of merged MERGED_FILE contents to
> > + avoid referencing a potentially absent file in
> FILE_INSTALL
> > + workqueue record. */
> > +
> > + const char *temp_dir_abspath;
> > + const char *temp_file_abspath;
> > + svn_stream_t *merged_contents;
> > + svn_stream_t *tmp_contents;
> > + apr_file_t *file;
> > +
> > + SVN_ERR(svn_io_file_open(, merged_file,
> > + APR_READ, APR_OS_DEFAULT,
> > + scratch_pool));
> > + merged_contents = svn_stream_from_aprfile2(file, FALSE,
> > + scratch_pool);
> > +
> > + SVN_ERR(svn_wc__db_temp_wcroot_tempdir(_dir_abspath,
> db,
> > + local_abspath,
> > + scratch_pool,
> > +
Re: [PATCH] Make sure resolving a conflict with `svn_wc_conflict_choose_merged' can't break the workqueue by referencing a non-existing file
On Thu, Apr 21, 2022 at 08:25:32PM +0300, Sergey Raevskiy wrote:
> I've noticed that in some cases a working copy can become inoperable
> after merge text conflict resolution with `svn_wc_conflict_choose_merged'
> option.
>
> The problem can occur in the following scenario:
>
> 1. There is a text conflict, that is resolved with
> `svn_wc_conflict_choose_merged' option and specifying some
> path as MERGED_FILE. According to the API it can be any file,
> let's say "D:\merged.txt".
>
> 2. Previous step adds a FILE_INSTALL item into workqueue with a
> reference to "D:\merged.txt" and then runs the workqueue.
>
> 3. An error happens when running the workqueue.
> For example a working file cannot be overwritten because
> of 'Access denied' error.
>
> 4. The "D:\merged.txt" file gets deleted for some reason.
>
> 5. At this point the working copy is inoperable and cannot be
> fixed by 'svn cleanup', because workqueue contains absolute
> path to non-existing file ("D:\merged.txt").
>
> I've attached a patch with fix for this problem. The patch contains a
> simplified regression test that reproduces problem by returning
> non-existing path from resolver callback.
>
> Log message:
> [[[
> Make sure resolving a conflict with `svn_wc_conflict_choose_merged' can't
> break the workqueue by referencing a non-existing file
>
> * subversion/libsvn_wc/conflicts.c
> (build_text_conflict_resolve_items): Create private copy of MERGED_FILE
>contents in case of `svn_wc_conflict_choose_merged' is specified.
>Doing that to avoid referencing a potentially absent file in FILE_INSTALL
>workqueue record.
>
> Patch by: sergey.raevskiy{_AT_}visualsvn.com
> ]]]
Thanks, Sergey! Your fix makes sense to me. One question below:
> Index: subversion/libsvn_wc/conflicts.c
> ===
> --- subversion/libsvn_wc/conflicts.c (revision 1900114)
> +++ subversion/libsvn_wc/conflicts.c (working copy)
> @@ -1706,9 +1706,49 @@ build_text_conflict_resolve_items(svn_skel_t **wor
> good to use". */
>case svn_wc_conflict_choose_merged:
> {
> - install_from_abspath = merged_file
> - ? merged_file
> - : local_abspath;
> + if (merged_file)
> +{
> + /* Create private copy of merged MERGED_FILE contents to
> + avoid referencing a potentially absent file in FILE_INSTALL
> + workqueue record. */
> +
> + const char *temp_dir_abspath;
> + const char *temp_file_abspath;
> + svn_stream_t *merged_contents;
> + svn_stream_t *tmp_contents;
> + apr_file_t *file;
> +
> + SVN_ERR(svn_io_file_open(, merged_file,
> + APR_READ, APR_OS_DEFAULT,
> + scratch_pool));
> + merged_contents = svn_stream_from_aprfile2(file, FALSE,
> + scratch_pool);
> +
> + SVN_ERR(svn_wc__db_temp_wcroot_tempdir(_dir_abspath, db,
> + local_abspath,
> + scratch_pool,
> + scratch_pool));
> +
> + SVN_ERR(svn_stream_open_unique(_contents,
> + _file_abspath,
> + temp_dir_abspath,
> + svn_io_file_del_none,
> + scratch_pool, scratch_pool));
> +
> + SVN_ERR(svn_stream_copy3(merged_contents, tmp_contents,
> + cancel_func, cancel_baton,
> + scratch_pool));
> +
> + install_from_abspath = temp_file_abspath;
Should we copy file permission bits as well as content?
For example, with the svn_io_copy_perms() function?
Or perhaps this entire copy operation could be done with svn_io_copy_file(),
and with the copy_perms parameter set to TRUE? If this is possible then the
code would be a lot simpler.
> +static svn_error_t *
> +test_resolve_choose_merged_non_existing_path(const svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
We could also verify matching file system permission bits in this test.
