You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2013/03/27 15:10:54 UTC
svn commit: r1461590 - in /subversion/trunk/subversion:
libsvn_diff/diff_file.c tests/libsvn_diff/diff-diff3-test.c
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,
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);
Re: svn commit: r1461590 - in /subversion/trunk/subversion:
libsvn_diff/diff_file.c tests/libsvn_diff/diff-diff3-test.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Mar 28, 2013 at 12:18 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Johan Corveleyn <jc...@gmail.com> writes:
>
>> 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):
>
> I extended the test and committed your patch in r1462041.
Great, thanks.
--
Johan
RE: svn commit: r1461590 - in /subversion/trunk/subversion: libsvn_diff/diff_file.c tests/libsvn_diff/diff-diff3-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: MARTIN PHILIP [mailto:codematters@ntlworld.com] On Behalf Of
> Philip Martin
> Sent: donderdag 28 maart 2013 12:18
> To: Johan Corveleyn
> Cc: philip@apache.org; dev@subversion.apache.org
> Subject: Re: svn commit: r1461590 - in /subversion/trunk/subversion:
> libsvn_diff/diff_file.c tests/libsvn_diff/diff-diff3-test.c
>
> Johan Corveleyn <jc...@gmail.com> writes:
>
> > 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):
>
> I extended the test and committed your patch in r1462041.
For the mailing list history:
13:02 <@Bert> philipm: Did you see the problem described by jcorvel in his
mail? Or is the fix+test to be on the safe
side?
13:02 < philipm> What problem exactly?
13:03 <@Bert> philipm: r1462041
13:04 < philipm> I see, what you are asking is did the previous fix still
allow invalid reads to occur?
13:04 <@Bert> *nod* Exactly
13:04 < philipm> Yes, the extended test causes invalid read with the
previous fix.
13:05 < philipm> The invalid read doesn't occur with the new fix.
Bert
Re: svn commit: r1461590 - in /subversion/trunk/subversion: libsvn_diff/diff_file.c tests/libsvn_diff/diff-diff3-test.c
Posted by Philip Martin <ph...@wandisco.com>.
Johan Corveleyn <jc...@gmail.com> writes:
> 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):
I extended the test and committed your patch in r1462041.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1461590 - in /subversion/trunk/subversion:
libsvn_diff/diff_file.c tests/libsvn_diff/diff-diff3-test.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Wed, Mar 27, 2013 at 3:10 PM, <ph...@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);
>
>