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. :)
Since both types are unsigned by definition, standard C type coercion
rules guarantee that the uintptr_t result holds the same value as the
original size_t, so there isn't really any type mismatch here.
But I can't help wondering if that bit of code could be written more
elegantly without all the casts.
-- Brane
Received on 2012-06-07 22:43:50 CEST