You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/06/24 21:19:57 UTC
Re: svn commit: r31868 - trunk/subversion/libsvn_client
glasser@tigris.org writes:
> Log:
> Fixes to pool usage in libsvn_client copy code. Specifically, prevent
> unbounded memory use when wc-to-wc copying or moving multiple files,
> by passing an iterpool to propagate_mergeinfo_within_wc.
>
> Found by: Matt McHenry <mm...@carnegielearning.com>
Aha! I knew it! :-)
Comments below...
> --- trunk/subversion/libsvn_client/copy.c (r31867)
> +++ trunk/subversion/libsvn_client/copy.c (r31868)
> @@ -371,7 +371,7 @@ do_wc_to_wc_copies(const apr_array_heade
> if (src_access)
> {
> err = propagate_mergeinfo_within_wc(pair, src_access, dst_access,
> - ctx, pool);
> + ctx, iterpool);
> if (err)
> break;
+1, but is there any reason not to do the same to the
svn_path_split(pair->src, &src_parent, NULL, pool);
...call earlier in that same loop? Well, I just did it in r31870.
> @@ -463,7 +463,7 @@ do_wc_to_wc_moves(const apr_array_header
> break;
>
> err = propagate_mergeinfo_within_wc(pair, src_access, dst_access,
> - ctx, pool);
> + ctx, iterpool);
> if (err)
> break;
Heh, in this case the svn_path_split() above is already using iterpool.
However, I'm still not sure why dst_access is treated differently
between those two functions. I've left it alone in r31870, but it bears
a second look.
> @@ -1773,7 +1773,7 @@ setup_copy(svn_commit_info_t **commit_in
> iterpool));
> src_basename = svn_path_basename(pair->src, iterpool);
> if (srcs_are_urls && ! dst_is_url)
> - src_basename = svn_path_uri_decode(src_basename, pool);
> + src_basename = svn_path_uri_decode(src_basename, iterpool);
>
> /* Check to see if all the sources are urls or all working copy
> * paths. */
*nod* But I really wonder if that function shouldn't use a subpool
where it currently uses pool, too...
> @@ -2015,11 +2015,11 @@ svn_client_copy4(svn_commit_info_t **com
>
> src_basename = svn_path_basename(src_path, subpool);
> if (svn_path_is_url(src_path) && ! svn_path_is_url(dst_path))
> - src_basename = svn_path_uri_decode(src_basename, pool);
> + src_basename = svn_path_uri_decode(src_basename, subpool);
...like this one does.
> err = setup_copy(&commit_info,
> sources,
> - svn_path_join(dst_path, src_basename, pool),
> + svn_path_join(dst_path, src_basename, subpool),
> FALSE /* is_move */,
> TRUE /* force, set to avoid deletion check */,
> make_parents,
> @@ -2033,7 +2033,7 @@ svn_client_copy4(svn_commit_info_t **com
> if (commit_info)
> *commit_info_p = svn_commit_info_dup(commit_info, pool);
> else
> - *commit_info_p = commit_info;
> + *commit_info_p = NULL;
> }
Yes, much more readable.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31868 - trunk/subversion/libsvn_client
Posted by David Glasser <gl...@davidglasser.net>.
On Tue, Jun 24, 2008 at 2:19 PM, Karl Fogel <kf...@red-bean.com> wrote:
> glasser@tigris.org writes:
>> Log:
>> Fixes to pool usage in libsvn_client copy code. Specifically, prevent
>> unbounded memory use when wc-to-wc copying or moving multiple files,
>> by passing an iterpool to propagate_mergeinfo_within_wc.
>>
>> Found by: Matt McHenry <mm...@carnegielearning.com>
>
> Aha! I knew it! :-)
>
> Comments below...
>
>> --- trunk/subversion/libsvn_client/copy.c (r31867)
>> +++ trunk/subversion/libsvn_client/copy.c (r31868)
>> @@ -371,7 +371,7 @@ do_wc_to_wc_copies(const apr_array_heade
>> if (src_access)
>> {
>> err = propagate_mergeinfo_within_wc(pair, src_access, dst_access,
>> - ctx, pool);
>> + ctx, iterpool);
>> if (err)
>> break;
>
> +1, but is there any reason not to do the same to the
>
> svn_path_split(pair->src, &src_parent, NULL, pool);
>
> ...call earlier in that same loop? Well, I just did it in r31870.
Sounds good.
>> @@ -463,7 +463,7 @@ do_wc_to_wc_moves(const apr_array_header
>> break;
>>
>> err = propagate_mergeinfo_within_wc(pair, src_access, dst_access,
>> - ctx, pool);
>> + ctx, iterpool);
>> if (err)
>> break;
>
> Heh, in this case the svn_path_split() above is already using iterpool.
>
> However, I'm still not sure why dst_access is treated differently
> between those two functions. I've left it alone in r31870, but it bears
> a second look.
Not sure. Hyrum?
>> @@ -1773,7 +1773,7 @@ setup_copy(svn_commit_info_t **commit_in
>> iterpool));
>> src_basename = svn_path_basename(pair->src, iterpool);
>> if (srcs_are_urls && ! dst_is_url)
>> - src_basename = svn_path_uri_decode(src_basename, pool);
>> + src_basename = svn_path_uri_decode(src_basename, iterpool);
>>
>> /* Check to see if all the sources are urls or all working copy
>> * paths. */
>
> *nod* But I really wonder if that function shouldn't use a subpool
> where it currently uses pool, too...
Eh, setup_copy's pool is the subpool from its caller (eg,
svn_client_copy4), and the caller doesn't allocate much itself. So
that's not really necessary.
--dave
>> @@ -2015,11 +2015,11 @@ svn_client_copy4(svn_commit_info_t **com
>>
>> src_basename = svn_path_basename(src_path, subpool);
>> if (svn_path_is_url(src_path) && ! svn_path_is_url(dst_path))
>> - src_basename = svn_path_uri_decode(src_basename, pool);
>> + src_basename = svn_path_uri_decode(src_basename, subpool);
>
> ...like this one does.
>
>> err = setup_copy(&commit_info,
>> sources,
>> - svn_path_join(dst_path, src_basename, pool),
>> + svn_path_join(dst_path, src_basename, subpool),
>> FALSE /* is_move */,
>> TRUE /* force, set to avoid deletion check */,
>> make_parents,
>> @@ -2033,7 +2033,7 @@ svn_client_copy4(svn_commit_info_t **com
>> if (commit_info)
>> *commit_info_p = svn_commit_info_dup(commit_info, pool);
>> else
>> - *commit_info_p = commit_info;
>> + *commit_info_p = NULL;
>> }
>
> Yes, much more readable.
>
> -K
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org