[PATCH] possible crash on svn revert

Hello.

Well, it is week-end and so I thougth it is the right time to assure
you have something to read on Monday... ;-)

Seriously, while browsing through bite-sized tasks, namely
http://subversion.tigris.org/issues/show_bug.cgi?id=714, I got a bit
side-tracked and stumbled upon crash bug.

Recipe:

  nora:~> cd /usr/local/src/phi/test
  nora:src/phi/test> svnadmin create repos2
  nora:src/phi/test> svn co file:///usr/local/src/phi/test/repos2 wc
  Checked out revision 0.
  nora:src/phi/test> cd wc
  nora:phi/test/wc> mkdir foo
  nora:phi/test/wc> svn add foo
  A foo
  nora:phi/test/wc> rm -rf foo
  nora:phi/test/wc> svn revert foo
  Segmentation fault (core dumped)
  Exit 139
  nora:phi/test/wc>

The problem is that libsvn_wc/adm_ops.c:svn_wc_revert() calls
svn_wc_remove_from_revision_control() with svn_wc_adm_access_t* set to
NULL, which is later (indirectly) dereferenced therein, therefore the
crash.

The bug is not easily fixed, because it is a new use case which is not
supported by the interface yet:

svn_error_t *
svn_wc_remove_from_revision_control (svn_wc_adm_access_t *adm_access,
                                     const char *name,
                                     svn_boolean_t destroy_wf,
                                     apr_pool_t *pool)

ADM_ACCESS is expected to be a valid write lock. The only valid
svn_wc_adm_access_t, which can be obtained is the one to the parent
directory of "foo", e.g. the root "". Then

  svn_wc_adm_access_path(ADM_ACCESS)

gives back the path to the root dir. Therefore somehow the name of
"foo" had to be passed to the function. But passing anything else than
SVN_WC_ENTRY_THIS_DIR for NAME is interpreted as the paramater being a
_file_. And of course the belonging functionality does not fit,
neither does the handling for directories (tries to delete the
directory given by ADM_ACCESS, here: the *root*).

A simply fix, which prevents the crash, but does not provide the
wished functionality would be:

Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c
+++ subversion/libsvn_wc/adm_ops.c 2002-09-21 18:22:01.000000000 +0200
@@ -1205,7 +1205,7 @@
         {
           SVN_ERR (svn_wc_adm_open (&parent_access, NULL, p_dir, TRUE, TRUE,
                                     pool));
- if (kind == svn_node_dir)
+ if (entry->kind == svn_node_dir)
             SVN_ERR (svn_wc_adm_retrieve (&dir_access, parent_access, path,
                                           pool));
           else

With this patch one gets:

  nora:phi/test/wc> svn revert foo
  subversion/libsvn_wc/lock.c:360: (apr_err=155005, src_err=0)
  svn: Working copy not locked
  svn: directory not locked (foo)
  Exit 1

The least intrusive way to get the it working, which I found found by
now, is to put the 3 required function calls into svn_wc_revert()
itself, instead of calling svn_wc_remove_from_revision_control(). No
need to say that I am not really happy with that solution.

But since I do not see a better fix at the moment, the patch below
will prevent the crash and make svn revert work as supposed in the
discussed case. It works for me and passed "make check".

Regards,

        Benjamin.

2002-09-21 Benjamin Pflugmann <benjamin-svn-cvs@pflugmann.de>

        * libsvn_wc/adm_ops.c:
        (svn_wc_delete): minor style change
        (svn_wc_revert): Fixed a crash.
        svn_wc_remove_from_revision_control may not be called without a
        valid lock. When trying to revert an added, but manually deleted
        directory, only the parent lock would be valid. But passing the
        parent lock would be interpreted as request to remove the parent.
        Therefore do remove the revision control entry without calling
        svn_wc_remove_from_revision_control at all.

Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c
+++ subversion/libsvn_wc/adm_ops.c 2002-09-21 18:30:47.000000000 +0200
@@ -617,7 +617,7 @@
   if (!entry)
     return erase_unversioned_from_wc (path, pool);
     
- was_schedule_add = entry->schedule == svn_wc_schedule_add;
+ was_schedule_add = (entry->schedule == svn_wc_schedule_add);
 
   if (entry->kind == svn_node_dir)
     {
@@ -1230,13 +1230,15 @@
          entry. */
       svn_boolean_t was_deleted = FALSE;
       const char *parent, *basey;
- svn_wc_adm_access_t *adm_access;
 
       svn_path_split_nts (path, &parent, &basey, pool);
       if (entry->kind == svn_node_file)
         {
           was_deleted = entry->deleted;
- adm_access = parent_access;
+
+ /* Remove the item from revision control. */
+ SVN_ERR (svn_wc_remove_from_revision_control
+ (parent_access, bname, FALSE, pool));
         }
       else if (entry->kind == svn_node_dir)
         {
@@ -1246,21 +1248,27 @@
           parents_entry = apr_hash_get (entries, basey, APR_HASH_KEY_STRING);
           if (parents_entry)
             was_deleted = parents_entry->deleted;
- adm_access = dir_access;
+
+ /* Remove the item from revision control. */
+ if( kind == svn_node_dir )
+ {
+ SVN_ERR (svn_wc_remove_from_revision_control
+ (dir_access, SVN_WC_ENTRY_THIS_DIR, FALSE, pool));
+ }
+ else
+ {
+ /* directory in WC does not exist (anymore), therefore
+ only remove revision information. */
+ SVN_ERR (svn_wc_adm_write_check (parent_access));
+ svn_wc__entry_remove (entries, basey);
+ SVN_ERR (svn_wc__entries_write (entries, parent, pool));
+ }
         }
       else /* Else it's `none', or something exotic like a symlink... */
         return svn_error_createf
           (SVN_ERR_NODE_UNKNOWN_KIND, 0, NULL, pool,
            "Unknown or unexpected kind for path %s", path);
 
- /* Remove the item from revision control. */
- if (entry->kind == svn_node_dir)
- SVN_ERR (svn_wc_remove_from_revision_control
- (adm_access, SVN_WC_ENTRY_THIS_DIR, FALSE, pool));
- else
- SVN_ERR (svn_wc_remove_from_revision_control (adm_access, bname,
- FALSE, pool));
-
       /* Recursivity is taken care of by svn_wc_remove_from_revision_control,
          and we've definitely reverted PATH at this point. */
       recursive = FALSE;