On Tue, Nov 15, 2011 at 9:59 AM, <julianfoad_at_apache.org> wrote:
> Author: julianfoad
> Date: Tue Nov 15 17:59:03 2011
> New Revision: 1202338
>
> URL: http://svn.apache.org/viewvc?rev=1202338&view=rev
> Log:
> * subversion/libsvn_client/merge.c
> (merge_cousins_and_supplement_mergeinfo, merge_locked): Don't allocate a
> structure in the pool, as it's only needed locally.
>
> Modified:
> subversion/trunk/subversion/libsvn_client/merge.c
>
> Modified: subversion/trunk/subversion/libsvn_client/merge.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1202338&r1=1202337&r2=1202338&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/merge.c (original)
> +++ subversion/trunk/subversion/libsvn_client/merge.c Tue Nov 15 17:59:03
> 2011
> @@ -9075,16 +9075,11 @@ merge_cousins_and_supplement_mergeinfo(c
> point-to-point merge... */
> if (! record_only)
> {
> - merge_source_t *faux_source;
> + merge_source_t faux_source = { URL1, rev1, URL2, rev2 };
> apr_array_header_t *faux_sources =
> apr_array_make(scratch_pool, 1, sizeof(merge_source_t *));
> modified_subtrees = apr_hash_make(scratch_pool);
> - faux_source = apr_pcalloc(scratch_pool, sizeof(*faux_source));
> - faux_source->url1 = URL1;
> - faux_source->url2 = URL2;
> - faux_source->rev1 = rev1;
> - faux_source->rev2 = rev2;
> - APR_ARRAY_PUSH(faux_sources, merge_source_t *) = faux_source;
> + APR_ARRAY_PUSH(faux_sources, merge_source_t *) = &faux_source;
> SVN_ERR(do_merge(&modified_subtrees, NULL, faux_sources,
> target_abspath,
> FALSE, TRUE, same_repos,
> ignore_ancestry, force, dry_run, FALSE, NULL, TRUE,
> @@ -9292,7 +9287,6 @@ merge_locked(const char *source1,
> svn_revnum_t youngest_rev = SVN_INVALID_REVNUM;
> svn_ra_session_t *ra_session1, *ra_session2;
> apr_array_header_t *merge_sources;
> - merge_source_t *merge_source;
> svn_error_t *err;
> svn_boolean_t use_sleep = FALSE;
> const char *yc_path = NULL;
> @@ -9494,13 +9488,9 @@ merge_locked(const char *source1,
> else
> {
> /* Build a single-item merge_source_t array. */
> + merge_source_t merge_source = { URL1, rev1, URL2, rev2 };
> merge_sources = apr_array_make(scratch_pool, 1,
> sizeof(merge_source_t *));
> - merge_source = apr_pcalloc(scratch_pool, sizeof(*merge_source));
> - merge_source->url1 = URL1;
> - merge_source->url2 = URL2;
> - merge_source->rev1 = rev1;
> - merge_source->rev2 = rev2;
> - APR_ARRAY_PUSH(merge_sources, merge_source_t *) = merge_source;
> + APR_ARRAY_PUSH(merge_sources, merge_source_t *) = &merge_source;
> }
>
This makes me a little nervous. I don't know what the compiler guarantees
are about something allocated on the stack in a sub-block within a
function, but the above use just looks wrong. As soon as the variable goes
out of scope, the compiler may choose to put something else on the stack in
its place, which in this case would lead to memory problems.
Perhaps the solution is to move the variable declaration back to the top of
the file.
I haven't checked the context to see if this is a problem in the other hunk
in this change.
-Hyrum
>
> /* Close our temporary RA sessions. */
>
>
>
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Received on 2011-11-15 19:21:12 CET