Re: [PATCH][merge-tracking]Data loss due to line buffered SAX stream on mergeinfo retrieval.

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