On 07.06.2012 12:16, Daniel Widenfalk wrote: > On 2012-06-07 11:47, Bert Huijben wrote: >> >>> -----Original Message----- >>> From: Bert Huijben [mailto:b...@qqmail.nl] >>> Sent: donderdag 7 juni 2012 11:34 >>> To: 'Daniel Widenfalk'; dev@subversion.apache.org; >>> us...@subversion.apache.org >>> Subject: RE: Potential issue in >> libsvn_diff:diff_file.c:find_identical_prefix >>> >>> >>>> -----Original Message----- >>>> From: Daniel Widenfalk [mailto:daniel.widenf...@iar.se] >>>> Sent: donderdag 7 juni 2012 11:06 >>>> To: dev@subversion.apache.org; us...@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
