You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2011/11/18 19:30:43 UTC
memory pool problem in merge
valgrind is giving "invalid read" during merge_tree_conflict_tests.py
test 22 at line 1735.
valgrind -q ../../svn/.libs/lt-svn merge file:///home/pm/sw/subversion/obj/subversion/tests/cmdline/svn-test-work/repositories/merge_tree_conflict_tests-22/A file:///home/pm/sw/subversion/obj/subversion/tests/cmdline/svn-test-work/repositories/merge_tree_conflict_tests-22/branch svn-test-work/working_copies/merge_tree_conflict_tests-22/A --config-dir /home/pm/sw/subversion/obj/subversion/tests/cmdline/svn-test-work/local_tmp/config --password rayjandom --no-auth-cache --username jrandom
==4683== Invalid read of size 1
==4683== at 0x5E70D7D: find_entry (apr_hash.c:265)
==4683== by 0x5E7111E: apr_hash_set (apr_hash.c:343)
==4683== by 0x4E6AA83: tree_conflict_on_add (merge.c:667)
==4683== by 0x4E6E739: merge_dir_added (merge.c:2292)
==4683== by 0x4E94912: add_directory (repos_diff.c:706)
==4683== by 0x5593BED: add_directory (cancel.c:115)
==4683== by 0x5593BED: add_directory (cancel.c:115)
==4683== by 0x6A64764: update_entry (reporter.c:927)
==4683== by 0x6A65574: delta_dirs (reporter.c:1210)
==4683== by 0x6A647F6: update_entry (reporter.c:931)
==4683== by 0x6A65574: delta_dirs (reporter.c:1210)
==4683== by 0x6A65AA6: drive (reporter.c:1276)
==4683== Address 0xb28e020 is 0 bytes inside a block of size 116 free'd
==4683== at 0x4C240FD: free (vg_replace_malloc.c:366)
==4683== by 0x5E79BB8: pool_clear_debug (apr_pools.c:1575)
==4683== by 0x5E79CFD: pool_destroy_debug (apr_pools.c:1637)
==4683== by 0x4E947E7: delete_entry (repos_diff.c:672)
==4683== by 0x5593B13: delete_entry (cancel.c:95)
==4683== by 0x5593B13: delete_entry (cancel.c:95)
==4683== by 0x6A64508: update_entry (reporter.c:901)
==4683== by 0x6A65574: delta_dirs (reporter.c:1210)
==4683== by 0x6A647F6: update_entry (reporter.c:931)
==4683== by 0x6A65574: delta_dirs (reporter.c:1210)
==4683== by 0x6A65AA6: drive (reporter.c:1276)
==4683== by 0x6A65F2E: finish_report (reporter.c:1340)
==4683==
This hack fixes the problem:
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1203766)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -593,6 +593,7 @@ tree_conflict(merge_cmd_baton_t *merge_b,
if (merge_b->conflicted_paths == NULL)
merge_b->conflicted_paths = apr_hash_make(merge_b->pool);
+ victim_abspath = apr_pstrdup(merge_b->pool, victim_abspath);
apr_hash_set(merge_b->conflicted_paths, victim_abspath,
APR_HASH_KEY_STRING, victim_abspath);
}
but I think the real fix is to identify which call to tree_conflict is
passing victim_path with the wrong lifetime.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: memory pool problem in merge
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 21, 2011 at 01:00:08PM +0000, Philip Martin wrote:
> Stefan Sperling <st...@elego.de> writes:
>
> > I think your patch makes sense as-is. This code creates the conflicted_paths
> > table and should ensure that anything referenced from the table has the
> > same lifetime as the table itself.
>
> I'm happy with that for tree_conflict. I think tree_conflict_on_add
> should do the same when it adds an entry to same hash.
Yes, I agree.
Re: memory pool problem in merge
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
> I think your patch makes sense as-is. This code creates the conflicted_paths
> table and should ensure that anything referenced from the table has the
> same lifetime as the table itself.
I'm happy with that for tree_conflict. I think tree_conflict_on_add
should do the same when it adds an entry to same hash.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: memory pool problem in merge
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 21, 2011 at 12:12:56PM +0000, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
> > This hack fixes the problem:
> >
> > Index: subversion/libsvn_client/merge.c
> > ===================================================================
> > --- subversion/libsvn_client/merge.c (revision 1203766)
> > +++ subversion/libsvn_client/merge.c (working copy)
> > @@ -593,6 +593,7 @@ tree_conflict(merge_cmd_baton_t *merge_b,
> > if (merge_b->conflicted_paths == NULL)
> > merge_b->conflicted_paths = apr_hash_make(merge_b->pool);
> >
> > + victim_abspath = apr_pstrdup(merge_b->pool, victim_abspath);
> > apr_hash_set(merge_b->conflicted_paths, victim_abspath,
> > APR_HASH_KEY_STRING, victim_abspath);
> > }
> >
> > but I think the real fix is to identify which call to tree_conflict is
> > passing victim_path with the wrong lifetime.
>
> So I have to fix two callers to get the test to pass under valgrind:
> merge_file_deleted:2078 and merge_dir_deleted:2424. There are lots of
> other callers of tree_conflict that don't appear to duplicate the path,
> including some in merge_file_deleted and merge_dir_deleted, and I don't
> need to change them to avoid valgrind error. This is partly because the
> regression tests don't cover all the code paths, but some of the other
> callers are invoked and don't have to change.
>
> So I don't know what to do. Is tree_conflict responsible for
> duplicating the path, or is the caller responsible? There is also
> tree_conflict_on_add which is very similar.
I think your patch makes sense as-is. This code creates the conflicted_paths
table and should ensure that anything referenced from the table has the
same lifetime as the table itself.
Re: memory pool problem in merge
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> This hack fixes the problem:
>
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 1203766)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -593,6 +593,7 @@ tree_conflict(merge_cmd_baton_t *merge_b,
> if (merge_b->conflicted_paths == NULL)
> merge_b->conflicted_paths = apr_hash_make(merge_b->pool);
>
> + victim_abspath = apr_pstrdup(merge_b->pool, victim_abspath);
> apr_hash_set(merge_b->conflicted_paths, victim_abspath,
> APR_HASH_KEY_STRING, victim_abspath);
> }
>
> but I think the real fix is to identify which call to tree_conflict is
> passing victim_path with the wrong lifetime.
So I have to fix two callers to get the test to pass under valgrind:
merge_file_deleted:2078 and merge_dir_deleted:2424. There are lots of
other callers of tree_conflict that don't appear to duplicate the path,
including some in merge_file_deleted and merge_dir_deleted, and I don't
need to change them to avoid valgrind error. This is partly because the
regression tests don't cover all the code paths, but some of the other
callers are invoked and don't have to change.
So I don't know what to do. Is tree_conflict responsible for
duplicating the path, or is the caller responsible? There is also
tree_conflict_on_add which is very similar.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com