Re: svn commit: r1461590 - in /subversion/trunk/subversion: libsvn_diff/diff_file.c tests/libsvn_diff/diff-diff3-test.c

On Wed, Mar 27, 2013 at 3:10 PM, <philip_at_apache.org> wrote:
> Author: philip
> Date: Wed Mar 27 14:10:54 2013
> New Revision: 1461590
>
> URL: http://svn.apache.org/r1461590
> Log:
> Fix issue 4339, diff suffix scanning invalid read at last chunk boundary.
>
> * subversion/libsvn_diff/diff_file.c
> (find_identical_suffix): Handle last chunk.
>
> * subversion/tests/libsvn_diff/diff-diff3-test.c
> (test_token_compare): Add comments, tweak allocation to match use (which
> doesn't change the underlying allocation due to alignment rounding).
>
> Modified:
> subversion/trunk/subversion/libsvn_diff/diff_file.c
> subversion/trunk/subversion/tests/libsvn_diff/diff-diff3-test.c
>
> Modified: subversion/trunk/subversion/libsvn_diff/diff_file.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/diff_file.c?rev=1461590&r1=1461589&r2=1461590&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_diff/diff_file.c (original)
> +++ subversion/trunk/subversion/libsvn_diff/diff_file.c Wed Mar 27 14:10:54 2013
> @@ -549,6 +549,14 @@ find_identical_suffix(apr_off_t *suffix_
> /* Prefix ended in last chunk, so we can reuse the prefix buffer */
> file_for_suffix[i].buffer = file[i].buffer;
> }
> + else if (!length[i] && file_for_suffix[i].chunk == file[i].chunk + 1)
> + {
> + /* Prefix ended at end of last chunk, so we can reuse the
> + prefix buffer */
> + file_for_suffix[i].chunk = file[i].chunk;
> + file_for_suffix[i].buffer = file[i].buffer;
> + length[i] = CHUNK_SIZE;
> + }
> else
> {
> /* There is at least more than 1 chunk,

Hi Philip,

Thanks for finding and fixing this. However, I think you only fixed it
for one particular case of "empty-last-chunkness": where
find_identical_prefix ended in last_chunk - 1.

I think the problem is still there if for instance one of the files is
exactly 262144 bytes (2 * CHUNK_SIZE), and there is no or very little
identical prefix (so prefix scanning stops in the first chunk (chunk
number 0). Suffix scanning will (as always) start at last_chunk ==
offset_to_chunk(262144) == 2, which is an empty chunk, but this is not
file[i].chunk + 1. I think if we'd write a test for that case, you'll
see your valgrind warning again.

I can spend more time on this in a week or two, but if you want to dig
into it sooner, I think a better fix would be something like this
(untested / uncompiled):

[[[
Index: diff_file.c
===================================================================
--- diff_file.c (revision 1461996)
+++ diff_file.c (working copy)
@@ -544,19 +544,18 @@ find_identical_suffix(apr_off_t *suffix_lines, str
       file_for_suffix[i].chunk =
         (int) offset_to_chunk(file_for_suffix[i].size); /* last chunk */
       length[i] = offset_in_chunk(file_for_suffix[i].size);
+ if (length[i] == 0)
+ {
+ /* last chunk is an empty chunk -> start at next-to-last chunk */
+ file_for_suffix[i].chunk = file_for_suffix[i].chunk - 1;
+ length[i] = CHUNK_SIZE;
+ }
+
       if (file_for_suffix[i].chunk == file[i].chunk)
         {
           /* Prefix ended in last chunk, so we can reuse the prefix buffer */
           file_for_suffix[i].buffer = file[i].buffer;
         }
- else if (!length[i] && file_for_suffix[i].chunk == file[i].chunk + 1)
- {
- /* Prefix ended at end of last chunk, so we can reuse the
- prefix buffer */
- file_for_suffix[i].chunk = file[i].chunk;
- file_for_suffix[i].buffer = file[i].buffer;
- length[i] = CHUNK_SIZE;
- }
       else
         {
           /* There is at least more than 1 chunk,
]]]

This will probably go wrong if the file is completely empty (then
file_for_suffix[i].chunk will be set to -1), but I think in that case
this code shouldn't be hit anyway (early exit in datasources_open if
one of the files is empty).

-- 
Johan
>
> Modified: subversion/trunk/subversion/tests/libsvn_diff/diff-diff3-test.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_diff/diff-diff3-test.c?rev=1461590&r1=1461589&r2=1461590&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_diff/diff-diff3-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_diff/diff-diff3-test.c Wed Mar 27 14:10:54 2013
> @@ -2571,6 +2571,7 @@ test_token_compare(apr_pool_t *pool)
>
>    diff_opts->ignore_space = svn_diff_file_ignore_space_all;
>
> +  /* CHUNK_SIZE bytes */
>    original = svn_stringbuf_create_ensure(chunk_size, pool);
>    while (original->len < chunk_size - 8)
>      {
> @@ -2578,7 +2579,8 @@ test_token_compare(apr_pool_t *pool)
>      }
>    svn_stringbuf_appendcstr(original, "    @@@\n");
>
> -  modified = svn_stringbuf_create_ensure(chunk_size, pool);
> +  /* CHUNK_SIZE+1 bytes, one ' ' more than original */
> +  modified = svn_stringbuf_create_ensure(chunk_size + 1, pool);
>    while (modified->len < chunk_size - 8)
>      {
>        svn_stringbuf_appendcstr(modified, pattern);
>
>