[PATCH] Prevent the modification of the URL [was 1.0.1 veto for r8959]

[makl <ma...@tigris.org>]

Philip Martin wrote:
> A failed switch to the non-existant URL modifies the working copy:
> 
> $ svnadmin create repo
> $ svn co file://`pwd`/repo wc
> $ svn sw file://`pwd`/repo/foo wc
> ../svn/subversion/libsvn_repos/reporter.c:845: (apr_err=160005)
> svn: Cannot replace a directory from within
> $ svn info wc | grep URL
> URL: file:///home/pm/sw/subversion/obj/repo/foo

This patch guarantees that the parameters of delta_dirs are checked
*before* open_root is called.

It is an addition to the patch from John Szakmeister to ensure that this
problem can't occur with other functions too.

[[[
Fix unintended modification of directory entry.

Patch from <ma...@tigris.org>

* subversion/libsvn_repos/reporter.c
      (drive): Check the parameters of delta_dirs before open_root is
      called.
]]]

Re: [PATCH] Prevent the modification of the URL [was 1.0.1 veto for r8959]

["C. Michael Pilato" <cm...@collab.net>]

Ben Collins-Sussman <su...@collab.net> writes:

> Greg Hudson wrote:
> 
> > > Perhaps it would be better if the switch editor
> > > modified the directory URL at close_directory time ("I have made the
> > > necessary changes to this dir to change its location" vs. "I plan to
> > > make the changes to this dir to change its location").
> 
> I don't agree... the whole design of the update-editor is based on the
> idea that open_root() and open_dir() immediately write out the new
> (revnum, URL) into the entries file with the 'incomplete' flag.  This is
> the reason checkouts/updates are restartable.
> 
> So the real issue here is:  who's in charge of validating the URL before
> the editor-driver actually begins to drive?
> 
> I think the spirit of makl's patch is just fine:  the reporter code can
> definitely do some sanity checking before beginning the editor drive. 
> The "old" txn-based reporter code used to do that, so makl's patch fixes
> a regression of some sort.
> 
> But in addition to makl's patch, I think we should also try to identify
> earlier opportunities to validate the URL as well.

+1.  

It's silly for the server (who actually has a chance in the world of
knowing that a given path doesn't exist in the repository) to tell the
client anything about that URL other than, "It doesn't exist".  And
open_root() is, in a sense, a form of confirmation that the parameters
of the switch/update/checkout were at least valid.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Prevent the modification of the URL [was 1.0.1 veto for r8959]

[Ben Collins-Sussman <su...@collab.net>]

Greg Hudson wrote:

> > Perhaps it would be better if the switch editor
> > modified the directory URL at close_directory time ("I have made the
> > necessary changes to this dir to change its location" vs. "I plan to
> > make the changes to this dir to change its location").

I don't agree... the whole design of the update-editor is based on the
idea that open_root() and open_dir() immediately write out the new
(revnum, URL) into the entries file with the 'incomplete' flag.  This is
the reason checkouts/updates are restartable.

So the real issue here is:  who's in charge of validating the URL before
the editor-driver actually begins to drive?

I think the spirit of makl's patch is just fine:  the reporter code can
definitely do some sanity checking before beginning the editor drive. 
The "old" txn-based reporter code used to do that, so makl's patch fixes
a regression of some sort.

But in addition to makl's patch, I think we should also try to identify
earlier opportunities to validate the URL as well.



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Prevent the modification of the URL [was 1.0.1 veto for r8959]

[makl <ma...@tigris.org>]

Greg Hudson wrote:

> I'm not convinced that drive() is doing anything wrong by calling
> open_root before generating an error; it hasn't told the editor to
> make any changes yet.

If open_root (and open_directory later) doesn't change the URL of the
directory, all file entries and maybe other things.

> Perhaps it would be better if the switch editor
> modified the directory URL at close_directory time ("I have made the
> necessary changes to this dir to change its location" vs. "I plan to
> make the changes to this dir to change its location").

Maybe. But this is a bigger task than just preventing the modification
of the URL.

> (A potentially larger issue: I'm not sure if an aborted switch
> operation can leave directories in a consistent state.

As far as I tested it the directory is in a consistent state. Sure some
entries are still at the source branch, but you can solve the problem
and retry the switch.

> I suppose it might be enough to set the start-empty flag on the directory
> between the open-dir and close-dir steps.)

This will affect all operations that use update under the hood.

> and could have explained a few things better.
My English is not very well and thus I have some problems to find good
descriptions. I will try to improve this.



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Prevent the modification of the URL [was 1.0.1 veto for r8959]

[Greg Hudson <gh...@MIT.EDU>]

> This patch guarantees that the parameters of delta_dirs are checked
> *before* open_root is called.

I'm not convinced that drive() is doing anything wrong by calling
open_root before generating an error; it hasn't told the editor to
make any changes yet.  Perhaps it would be better if the switch editor
modified the directory URL at close_directory time ("I have made the
necessary changes to this dir to change its location" vs. "I plan to
make the changes to this dir to change its location").

(A potentially larger issue: I'm not sure if an aborted switch
operation can leave directories in a consistent state.  I suppose it
might be enough to set the start-empty flag on the directory between
the open-dir and close-dir steps.)

I also feel the patch could be better stylistically; you left the code
with an "if (a) { if (b) c; }" construct, didn't take the opportunity
to remove the braces around a single-line statement, and could have
explained a few things better.  Here is my take on the same change.

---
Fix unintended modification of directory entry during "switch".

Patch from <ma...@tigris.org>, modified by <gh...@mit.edu>.

* subversion/libsvn_repos/reporter.c
  (drive): When anchor is target and the source and destination aren't
   both directories, error out before calling open_root.

Index: reporter.c
===================================================================
--- reporter.c	(revision 9078)
+++ reporter.c	(working copy)
@@ -835,19 +835,21 @@
   if (info_is_set_path && !s_entry)
     s_fullpath = NULL;
 
+  /* If the anchor is the operand, the source and target must be dirs.
+     Check this before opening the root to avoid modifying the wc. */
+  if (!*b->s_operand && (!s_entry || s_entry->kind != svn_node_dir
+                         || !t_entry || t_entry->kind != svn_node_dir))
+    return svn_error_create (SVN_ERR_FS_PATH_SYNTAX, NULL,
+                             "Cannot replace a directory from within");
+
   SVN_ERR (b->editor->open_root (b->edit_baton, s_rev, pool, &root_baton));
 
+  /* If the anchor is the operand, diff the two directories; otherwise
+     update the operand within the anchor directory. */
   if (!*b->s_operand)
-    {
-      /* The wc anchor is the operand, so just diff the two directories. */
-      if (!s_entry || s_entry->kind != svn_node_dir
-          || !t_entry || t_entry->kind != svn_node_dir)
-        return svn_error_create (SVN_ERR_FS_PATH_SYNTAX, NULL,
-                                 "Cannot replace a directory from within");
-      SVN_ERR (delta_dirs (b, s_rev, s_fullpath, b->t_path, root_baton,
-                           "", info->start_empty, pool));
-    }
-  else  /* Update the operand within the anchor directory. */
+    SVN_ERR (delta_dirs (b, s_rev, s_fullpath, b->t_path, root_baton,
+                         "", info->start_empty, pool));
+  else
     SVN_ERR (update_entry (b, s_rev, s_fullpath, s_entry, b->t_path,
                            t_entry, root_baton, b->s_operand,
                            (info_is_set_path) ? NULL : info, TRUE, pool));

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org