Re: svn commit: r1485848 - /subversion/trunk/subversion/libsvn_subr/subst.c

On 24.05.2013 15:10, Stefan Sperling wrote:
> On Thu, May 23, 2013 at 08:55:32PM -0000, stefan2_at_apache.org wrote:
>> Author: stefan2
>> Date: Thu May 23 20:55:32 2013
>> New Revision: 1485848
>>
>> URL: http://svn.apache.org/r1485848
>> Log:
>> Double the speed translate_chunk in case that keyword substitution has
>> not been enabled.
> I believe this revision is causing crashes in my client built
> from trunk. The trunk client is also corrupting working copies I use.
>
> I'll investigate a bit further, but if my testing shows that this
> change is reponsible I am going to revert it ASAP to prevent the
> damage from spreading.
>
> 'svn diff' coredumps with:
>
> (gdb)
> #0 0x000011bd8dc946bd in svn_eol__find_eol_start (buf=0x11bd84838ffa "",
> len=12309) at subversion/libsvn_subr/eol.c:56
> 56 apr_uintptr_t chunk = *(const apr_uintptr_t *)buf;
> (gdb)
> Bottom (i.e., innermost) frame selected; you cannot go down.
> (gdb) p buf
> $1 = 0x11bd84838ffa ""
> (gdb) p chunk
> $2 = 0
> (gdb) p * buf
> $3 = 0 '\0'
>
> Some working files contain binary garbage after the crash.
> The working copy needs to be checked out again with a working client.
>
>> * subversion/libsvn_subr/subst.c
>> (translate_chunk): use a fast scanner if we only care about newlines
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_subr/subst.c
>>
>> Modified: subversion/trunk/subversion/libsvn_subr/subst.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/subst.c?rev=1485848&r1=1485847&r2=1485848&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_subr/subst.c (original)
>> +++ subversion/trunk/subversion/libsvn_subr/subst.c Thu May 23 20:55:32 2013
>> @@ -50,6 +50,7 @@
>> #include "svn_private_config.h"
>>
>> #include "private/svn_string_private.h"
>> +#include "private/svn_eol_private.h"
>>
>> /**
>> * The textual elements of a detranslated special file. One of these
>> @@ -1116,28 +1117,39 @@ translate_chunk(svn_stream_t *dst,
>> /* skip current EOL */
>> len += b->eol_str_len;
>>
>> - /* Check 4 bytes at once to allow for efficient pipelining
>> - and to reduce loop condition overhead. */
>> - while ((p + len + 4) <= end)

If the above condition is correct ...
[...]

>> + else
>> + {
>> + /* use our optimized sub-routine to find the next EOL */
>> + const char *eol
>> + = svn_eol__find_eol_start((char *)p + len, end - p);

... then the second parameter here should be (end - p + 1), not (end - p).

>> + len += (eol ? eol : end) - (p + len);

And I don't understand this statement at all.

-- Brane

-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com