>> --- subversion/libsvn_ra_dav/mergeinfo.c (revision 22315)
>> +++ subversion/libsvn_ra_dav/mergeinfo.c (working copy)
>> @@ -127,6 +127,8 @@
>> struct mergeinfo_baton *mb = baton;
>> apr_size_t nlen = len;
>> + char tmpc;
>> + char *mutable_cdata = (char*)cdata;
>> switch (state)
>> @@ -134,7 +136,13 @@
>> mb->curr_path = apr_pstrndup(mb->pool, cdata, nlen);
>> 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
len is 16(including the new line).
so setting cdata = '\0' sets the overwrites a char next to '\n'.
> Looks like an attempt to avoid copying "cdata"...
>> + mutable_cdata[len]='\0';
> We did it again, only this time we clobbered something else's memory.
> This time, we used "len" rather than "nlen" as our offset variable.
> Which we use doesn't really matter, but we should be consistent.
> Spacing is also missing from around the "=" character.
Sorry for the confusion.
>> + mb->curr_info = apr_pstrcat(mb->pool, mb->curr_info, cdata, NULL);
> What happens when mb->curr_info hasn't been set yet?
Yes realised the same and sent the followup patch on the same. Anyway
you fixed the same.
>> + mutable_cdata[nlen] = tmpc;
> Now we attempt to restore the data we previously clobbered, again
> changing the offset variable we're using.
Yes I agree that 'inconsistent variable usage is bad'.
Are you ok if I give a patch which uses a 'memory clobbering+restoration
approach' with consistent 'len or nlen not both'?
To unsubscribe, e-mail: firstname.lastname@example.org
For additional commands, e-mail: email@example.com
Received on Tue Nov 21 07:26:03 2006