On 06/07/2012 10:43 PM, Branko ÄŒibej wrote:
> On 07.06.2012 12:16, Daniel Widenfalk wrote:
>> On 2012-06-07 11:47, Bert Huijben wrote:
>>>> -----Original Message-----
>>>> From: Bert Huijben [mailto:bert_at_qqmail.nl]
>>>> Sent: donderdag 7 juni 2012 11:34
>>>> To: 'Daniel Widenfalk'; dev_at_subversion.apache.org;
>>>> users_at_subversion.apache.org
>>>> Subject: RE: Potential issue in
>>> libsvn_diff:diff_file.c:find_identical_prefix
>>>>
>>>>> -----Original Message-----
>>>>> From: Daniel Widenfalk [mailto:Daniel.Widenfalk_at_iar.se]
>>>>> Sent: donderdag 7 juni 2012 11:06
>>>>> To: dev_at_subversion.apache.org; users_at_subversion.apache.org
>>>>> Subject: Potential issue in
>>> libsvn_diff:diff_file.c:find_identical_prefix
>>>>> Hi,
>>>>>
>>>>> First off: I'm sorry if I post this in the wrong way.
>>>>>
>>>>> I've found a potential issue in the function "find_identical_prefix"
>>>>> in libsvn_diff/diff_file.c
>>>>>
>>>>> The faulty code looks like this:
>>>>>
>>>>> diff_file.c:432 (as per 1.7.1, code identical to 1.7.5)
>>>>> -------------------------------
>>>>> is_match = TRUE;
>>>>> for (delta = 0; delta< max_delta&& is_match; delta +=
>>>>> sizeof(apr_uintptr_t))
>>>>> {
>>>>> apr_uintptr_t chunk = *(const apr_size_t *)(file[0].curp +
>>>> delta);
>>>>> if (contains_eol(chunk))
>>>>> break;
>>>>>
>>>>> for (i = 1; i< file_len; i++)
>>>>> if (chunk != *(const apr_size_t *)(file[i].curp + delta))
>>>>> {
>>>>> is_match = FALSE;
>>>>> delta -= sizeof(apr_size_t);
>>>>> break;
>>>>> }
>>>>> }
>>>>> -------------------------------
>>>>>
>>>>> The problem is that the 64-bit build I'm using (ColabNet) have
>>>>> different sizes for apr_uintptr_t and apr_size_t.
>>>>>
>>>>> From looking at the disassembly I can deduce that
>>>>> sizeof(apr_uintptr_t) = 4 and sizeof(apr_size_t) = 8. This leads
>>>>> to these two issues:
>>>>>
>>>>> 1) Data is truncated in the initial read-up to "chunk" and the compare
>>>>> in the inner loop will fail because the second read-up will compare
>>>>> a 64-bit value against a 32-bit value.
>>>>>
>>>>> 2) When the test fails it will back up delta by 8, not 4, resulting in
>>>>> a buffer advance of -4 later in the code. Once the search code has
>>>>> advanced another 4 character if will be back at the same spot.
>>>>>
>>>>> Rinse and repeat.
>>>>>
>>>>> Are these a known issues?
>>>>>
>>>>> In my case this results in an infinite loop on the following input
>>>>> string:
>>>>>
>>>>> 23 0a 23 20 54 68 69 73 20 70 72 6f 6a 65 63
>>>>>
>>>>> I found this out when my svn-client spiraled into an infinite loop
>>>>> and would not respond to ctrl-c during a "svn up" command.
>>>> Which apr version did you use?
>>>>
>>>> I think this issue was fixed in Subversion 1.7.2:
>>>>
>>>> (From http://svn.apache.org/repos/asf/subversion/tags/1.7.2/CHANGES)
>>>> * properly define WIN64 on Windows x64 builds (r1188609)
>>>>
>>>> Not by a code change here in this piece of the code, but by making sure
>>> that
>>>> the pointer size is defined correctly in apr.
>>>> So that would fix this issue and maybe several others.
>>> And since then then also APR was patched to properly check for _WIN64
>>> instead of WIN64 for defining these variable types correctly.
>> Updating to 1.7.5 makes the issue go away. The type mismatch is still
>> in the 1.7.5 source but if the system do guarantee that
>>
>> sizeof(apr_uintptr_t) == sizeof(apr_size_t)
>>
>> then it should be ok.
> Unless the platform ABI is totally broken, then it should always be the
> case that sizeof(uintptr_t)>= sizeof(size_t), and the same holds for
> the APR aliases; assuming of course that they're defined correctly. :)
r1348472 should eliminate that inconsistency.
-- Stefan^2.
Received on 2012-06-09 20:36:55 CEST