Daniel Rall wrote:
> On Tue, 21 Nov 2006, Kamesh Jayachandran wrote:
>
>
>>>> --- subversion/libsvn_ra_dav/mergeinfo.c (revision 22315)
>>>> +++ subversion/libsvn_ra_dav/mergeinfo.c (working copy)
>>>> @@ -127,6 +127,8 @@
>>>>
> ...
>
>>>> @@ -134,7 +136,13 @@
>>>> mb->curr_path = apr_pstrndup(mb->pool, cdata, nlen);
>>>> break;
>>>> case ELEM_merge_info_info:
>>>> - mb->curr_info = apr_pstrndup(mb->pool, cdata, nlen);
>>>> + /* if the data is having multiple lines we need to concatenate the
>>>> + * current data with the previous one.
>>>> + */
>>>> + tmpc = mutable_cdata[nlen];
>>>>
>>> This will index off the end of the "mutable_cdata" string, which is
>>> only "len" character long, rather than the "len + 1" characters this
>>> would require.
>>>
>> It is 'len' not 'len + 1'.
>> Say for data
>> '/trunk/abc:1-15....something_else_we_should_not_bother........................'
>> len is 16(including the new line).
>>
>
> Are you trying to represent a piece of memory here? cdata[16] would
> be the 17th character in the string, which is one character off of the
> end of the memory which we're supposed to have access to in this
> context.
>
>
>> so setting cdata[16] = '\0' sets the overwrites a char next to '\n'.
>>
>
> Yeah, one character past the newline.
>
>
>>> Looks like an attempt to avoid copying "cdata"...
>>>
>> Yes.
>>
>
> I took a stab at making this more efficient -- would you test out this
> patch?
>
> Index: subversion/libsvn_ra_dav/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_ra_dav/mergeinfo.c (revision 22373)
> +++ subversion/libsvn_ra_dav/mergeinfo.c (working copy)
> @@ -137,9 +137,12 @@
> case ELEM_merge_info_info:
> if (mb->curr_info)
> {
> - char *to_append = apr_pstrmemdup(mb->pool, cdata, len);
> - mb->curr_info = apr_pstrcat(mb->pool, mb->curr_info, to_append,
> - NULL);
> + struct iovec fragments[] =
> + {
> + { (void *) mb->curr_info, strlen(mb->curr_info) },
> + { (void *) cdata, len }
> + };
> + mb->curr_info = apr_pstrcatv(mb->pool, fragments, 2, NULL);
> }
> else
> {
>
>
> ...
>
>> Are you ok if I give a patch which uses a 'memory clobbering+restoration
>> approach' with consistent 'len or nlen not both'?
>>
>
> I'm assuming that we shouldn't be touching memory past the end of
> "cdata[len - 1]", as the API semantics of our SAX parser don't imply
> we have any business with it...something like the patch I provided
> above seems like a safer and more clear way to go.
>
> - Dan
>
Now I have another thought. Why not use string streams? which should be
more clean and efficient way to address the problem?
Will post a patch soon.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Nov 21 09:35:29 2006