You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2007/09/27 23:27:33 UTC
Re: svn commit: r26813 - trunk/subversion/libsvn_client
On Thu, 27 Sep 2007, pburba@tigris.org wrote:
...
> Follow-up to r26803, fix some potential segfaults.
>
> * subversion/libsvn_client/merge.c
> (drive_merge_report_editor): Be consistent with our assumption that
> drive_merge_report_editor may be NULL and check for that.
> (svn_client_merge_peg3): Initialize children_with_mergeinfo.
...
> --- trunk/subversion/libsvn_client/merge.c (original)
> +++ trunk/subversion/libsvn_client/merge.c Thu Sep 27 12:38:58 2007
> @@ -1623,7 +1623,7 @@
>
> if (merge_b->target_has_dummy_merge_range)
> default_start = end;
> - else if (children_with_mergeinfo)
> + else if (children_with_mergeinfo && children_with_mergeinfo->nelts > 0)
> {
> svn_client__merge_path_t *target_merge_path_t;
> target_merge_path_t = APR_ARRAY_IDX(children_with_mergeinfo, 0,
> @@ -3797,6 +3797,8 @@
> }
> else
> {
> + children_with_mergeinfo =
> + apr_array_make(pool, 0, sizeof(svn_client__merge_path_t *));
> SVN_ERR(do_merge(URL1,
> range.start,
> URL2,
Hey Paul, why was this second case necesssary (e.g. where was the
segfault)? drive_merge_report_editor() seems to handle a NULL hash
okay, as does cleanup_noop_merge(). I didn't catch any other
consumers of this hash. It'd be nice to drop the NULL initialization
when declaring children_with_mergeinfo in svn_client_merge3(); it's
already absent in svn_client_merge_peg3().
Thanks, Dan
Re: svn commit: r26813 - trunk/subversion/libsvn_client
Posted by Kamesh Jayachandran <ka...@collab.net>.
>> --- trunk/subversion/libsvn_client/merge.c (original)
>> +++ trunk/subversion/libsvn_client/merge.c Thu Sep 27 12:38:58 2007
>> @@ -1623,7 +1623,7 @@
>>
>> if (merge_b->target_has_dummy_merge_range)
>> default_start = end;
>> - else if (children_with_mergeinfo)
>> + else if (children_with_mergeinfo && children_with_mergeinfo->nelts > 0)
>>
No need to check 'children_with_mergeinfo->nelts > 0' because of
'children_with_mergeinfo' is non-Null, it would be non-empty.
I marked this requirement in the docstring of
'drive_merge_report_editor' in r26822.
<snip from docstring of get_mergeinfo_paths>
*CHILDREN_WITH_MERGEINFO is guaranteed to be non-empty with its first
member being 'target' itself.
</snip>
>> {
>> svn_client__merge_path_t *target_merge_path_t;
>> target_merge_path_t = APR_ARRAY_IDX(children_with_mergeinfo, 0,
>> @@ -3797,6 +3797,8 @@
>> }
>> else
>> {
>> + children_with_mergeinfo =
>> + apr_array_make(pool, 0, sizeof(svn_client__merge_path_t *));
>> SVN_ERR(do_merge(URL1,
>> range.start,
>> URL2,
>>
>
> Hey Paul, why was this second case necesssary (e.g. where was the
> segfault)? drive_merge_report_editor() seems to handle a NULL hash
> okay, as does cleanup_noop_merge(). I didn't catch any other
> consumers of this hash. It'd be nice to drop the NULL initialization
> when declaring children_with_mergeinfo in svn_client_merge3(); it's
> already absent in svn_client_merge_peg3().
>
> Thanks, Dan
>
I addressed this too in r26822.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn commit: r26813 - trunk/subversion/libsvn_client
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Daniel Rall [mailto:dlr@collab.net]
> Sent: Thursday, September 27, 2007 7:28 PM
> To: Paul Burba
> Cc: dev@subversion.tigris.org
> Subject: Re: svn commit: r26813 - trunk/subversion/libsvn_client
>
> On Thu, 27 Sep 2007, pburba@tigris.org wrote:
> ...
> > Follow-up to r26803, fix some potential segfaults.
> >
> > * subversion/libsvn_client/merge.c
> > (drive_merge_report_editor): Be consistent with our
> assumption that
> > drive_merge_report_editor may be NULL and check for that.
> > (svn_client_merge_peg3): Initialize children_with_mergeinfo.
> ...
> > --- trunk/subversion/libsvn_client/merge.c (original)
> > +++ trunk/subversion/libsvn_client/merge.c Thu Sep 27 12:38:58 2007
> > @@ -1623,7 +1623,7 @@
> >
> > if (merge_b->target_has_dummy_merge_range)
> > default_start = end;
> > - else if (children_with_mergeinfo)
> > + else if (children_with_mergeinfo &&
> children_with_mergeinfo->nelts
> > + > 0)
> > {
> > svn_client__merge_path_t *target_merge_path_t;
> > target_merge_path_t =
> APR_ARRAY_IDX(children_with_mergeinfo, 0,
> > @@ -3797,6 +3797,8 @@
> > }
> > else
> > {
> > + children_with_mergeinfo =
> > + apr_array_make(pool, 0,
> sizeof(svn_client__merge_path_t
> > + *));
> > SVN_ERR(do_merge(URL1,
> > range.start,
> > URL2,
>
> Hey Paul, why was this second case necesssary (e.g. where was
> the segfault)? drive_merge_report_editor() seems to handle a
> NULL hash okay,
Hi Dan,
It didn't at the time, line 1629 in drive_merge_report_editor(),
target_merge_path_t = APR_ARRAY_IDX(children_with_mergeinfo, 0,
svn_client__merge_path_t *);
would operate on an uninitialized children_with_mergeinfo array in the
case I described here: http://svn.haxx.se/dev/archive-2007-09/0706.shtml
To your point, with the first fix above that couldn't happen any longer.
I was only being overly cautious since the same check was made later on
in the function. Anyhow, as I'm sure you saw, Kamesh addressed all of
this in r26822, so it's a moot point now.
Paul
> as does cleanup_noop_merge(). I didn't catch
> any other consumers of this hash. It'd be nice to drop the
> NULL initialization when declaring children_with_mergeinfo in
> svn_client_merge3(); it's already absent in svn_client_merge_peg3().
>
> Thanks, Dan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org