Re: segfault when deleting folders in 1.6.2

Paul Burba wrote:
> On Sun, May 31, 2009 at 7:35 AM, Stefan Küng <tortoisesvn_at_gmail.com> wrote:
>> On Thu, May 21, 2009 at 10:22, Stefan Küng <tortoisesvn_at_gmail.com> wrote:
>>> Hi,
>>>
>>> In the file libsvn_wc/adm_ops.c, function
>>> svn_wc_remove_from_revision_control(), line 2628:
>>>
>>> dir_entry = apr_hash_get(parent_entries, base_name,
>>> APR_HASH_KEY_STRING);
>>> if (dir_entry->depth != svn_depth_exclude)
>>> {
>>> svn_wc__entry_remove(parent_entries, base_name);
>>> SVN_ERR(svn_wc__entries_write(parent_entries, parent_access, pool));
>>>
>>> }
>>>
>>> the 'dir_entry' is NULL, and accessing it in the if-clause causes a
>>> segfault.
>>> From the crash dump I got, I can't see anything special about the path
>>> (it was F:\dev\svn\name\name), so no root path, no UNC path or anything
>>> else special I can think of.
>>>
>>> But the delete was called with --keep-local, so maybe that has something
>>> to do with it?
>> Some more information:
>> the folder which should be deleted is tree-conflicted (deleted in
>> repository, still present locally).
>>
>> Stefan

Actually, it's only an obstructed path. It's not like it was deleted in the
repos vs. edited in the WC. No, the deleted subtree is now out of version
control, they are "some other files lying around", edits don't matter as far
as Subversion is concerned. This will only become a tree-conflict if the
user tries to subsequently do an update or merge that involves this path,
i.e. add a new file/dir with the same name (sbutler,stsp: other cases??).

>
> Attached is a patch that adds a new update test to replicate this
> segfault, along with the very simple fix to address it.
>
> I'm going to hold off on committing though for two reasons:
>
> First, while this fixes the problem for 1.6.x, when running this new
> test on trunk it triggers an assert on line 2051 in
> entries.c:write_entry():
>
> 2048 if (entry->keep_local)
> 2049 {
> 2050 SVN_ERR_ASSERT(working_node != NULL);
> 2051 SVN_ERR_ASSERT(entry->schedule == svn_wc_schedule_delete);
> 2052 working_node->keep_local = TRUE;
> 2053 }

<rant>Whoa, a simple NULL check affects an assert like this??
It implies that previously, the "if" condition that you change in the patch
evaluated a NULL pointer and did ugly stuff, if I'm following correctly.

So it's probably the first time that this part of the code gets proper input
for this particular situation. So, hopefully the assert is wrong. Otherwise,
it sounds like a rat's tail "missing feature".</rant>

>
> This assert was added in r36105, but I can't figure out what its
> purpose is.

quoting include/svn_wc.h svn_entry_t
[[[
  /** Whether a local copy of this entry should be kept in the working copy
   * after a deletion has been committed, Only valid for the this-dir entry
   * when it is scheduled for deletion.
   *
   * @since New in 1.5. */
  svn_boolean_t keep_local;
]]]

So, the assert is verifying that keep_local is only set on an entry that is
also scheduled for deletion; So that every keep_local that is set is in fact
carried out and doesn't linger past the next commit, I think.

> Putting aside tree conflicts for a moment we can trigger
> this assert by simply deleting a path with a locally added subtree,
> e.g.:
>
> trunk_at_37912>svn st -v
> 1 1 jrandom .
> 1 1 jrandom A
> 1 1 jrandom A\mu
> 1 1 jrandom A\B
> 1 1 jrandom A\B\lambda
> 1 1 jrandom A\B\E
> 1 1 jrandom A\B\E\alpha
> 1 1 jrandom A\B\E\beta
> 1 1 jrandom A\B\F
> 1 1 jrandom A\C
> 1 1 jrandom A\D
> 1 1 jrandom A\D\gamma
> 1 1 jrandom A\D\G
> 1 1 jrandom A\D\G\rho
> 1 1 jrandom A\D\G\pi
> 1 1 jrandom A\D\G\tau
> 1 1 jrandom A\D\H
> 1 1 jrandom A\D\H\chi
> 1 1 jrandom A\D\H\omega
> 1 1 jrandom A\D\H\psi
> 1 1 jrandom iota
>
> trunk_at_37912>svn copy A\D A\C
> A A\C\D
>
> trunk_at_37912>svn delete A\C --keep-local
> D A\C\D\gamma
> D A\C\D\G\rho
> D A\C\D\G\pi
> D A\C\D\G\tau
> D A\C\D\G
> D A\C\D\H\chi
> D A\C\D\H\omega
> D A\C\D\H\psi
> D A\C\D\H
> ..\..\..\subversion\libsvn_wc\wc_db.c:2240: (apr_err=235000)
> svn: In file '..\..\..\subversion\libsvn_wc\entries.c' line 2051:
> assertion failed (entry->schedule == svn_wc_schedule_delete)
What with the backslashes ;)

Hm, would be nice to know on which entry this assertion fails.
But I'm already getting a notion that the cause is this:

The entry, say A/C, is scheduled for addition. It isn't actually added yet.
Then we go on deleting it, before it's a real repository entry. That's more
like a revert, not a delete, right? However, --keep-local sets keep_local on
it anyways (my guess). The result is a directory node that is *not*
scheduled for deletion but still has keep_local set. Probably just another
special case in an if statement somewhere.

Hey, I'm right, this says it all:

libsvn_wc/adm_ops.c:923 (mark_tree)
[[[
    /* Uncommitted directories (schedule add) that are to be scheduled for
     deletion are a special case, they don't need to be changed as they
     will be removed from their parent's entry list. */
  if (! (entry->schedule == svn_wc_schedule_add
         && schedule == svn_wc_schedule_delete))
  {
    if (modify_flags & SVN_WC__ENTRY_MODIFY_SCHEDULE)
      {
        tmp_entry.schedule = schedule;
        this_dir_flags |= SVN_WC__ENTRY_MODIFY_SCHEDULE;
      }

    if (modify_flags & SVN_WC__ENTRY_MODIFY_COPIED)
      {
        tmp_entry.copied = copied;
        this_dir_flags |= SVN_WC__ENTRY_MODIFY_COPIED;
      }
  }

  /* Set keep_local on the "this dir", if requested. */
  if (modify_flags & SVN_WC__ENTRY_MODIFY_KEEP_LOCAL)
    {
      tmp_entry.keep_local = keep_local;
      this_dir_flags |= SVN_WC__ENTRY_MODIFY_KEEP_LOCAL;
    }
]]]

I think all that needs to happen is that the keep_local part has to move
into the "if(! (..._add && ..._delete)){" scope.

>
> Hyrum, I'm ccing you in the hope you can explain the purpose of this
> assert (apologies if I'm being dense!).

:)

> ~~~~~
>
> Second, I'm wondering what additional limitations we should consider
> putting on operations on a tree conflicted directory before that TC is
> resolved or reverted. Obviously we can't commit a tree conflicted WC,
> but should we allow other operations, copies, moves, deletes, etc., of
> WC paths which are either a subtree of a tree conflicted directory or
> have a subtree that is tree conflicted. Or should all this be allowed
> and should it "work" (however that might be defined)? I'm CCing you
> TC guys in the hopes of prying some wisdom from your brains on this
> ;-)

This fix is not really that closely related to tree-conflicts, as pointed
out above. What operations are or are not allowed on tree-conflicted entries
is a matter of resolve strategies ... which we haven't really mapped out
completely yet, AFAIR. We should have some document on what operations do
what, I guess. So far, I only know of some TC resolving examples.
(beware, slightly outdated)
http://svn.collab.net/viewvc/svn/trunk/notes/tree-conflicts/use-cases-resolution.txt

Anyway. Don't worry. :)

~Neels

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2358939