Re: [Qemu-devel] [PATCH v3] linux-user: fix preadv/pwritev offsets

[Laurent Vivier]

Le 05/04/2018 à 18:27, Max Filippov a écrit :
> On Thu, Apr 5, 2018 at 8:52 AM, Laurent Vivier <address@hidden> wrote:
>> Why don't you try to de-construct then re-construct the offset?
> 
> It would require 128-bit arithmetic on 64-bit host.
> 
>> Kernel commit
>>   601cc11d054a "Make non-compat preadv/pwritev use native register size"
>> is interesting.
>>
>> static inline abi_llong target_pos_from_hilo(abi_ulong high,
>>                                              abi_ulong low)
>> {
>> #define TARGET_HALF_LONG_BITS (TARGET_LONG_BITS / 2)
>>    return (((abi_llong)high << TARGET_HALF_LONG_BITS) <<
>>                              TARGET_HALF_LONG_BITS) | low;
>> }
>>
>> #define HOST_HALF_LONG_BITS (HOST_LONG_BITS / 2)
>>
>>  abi_llong pos = target_pos_from_hilo(arg4, arg5);
>>  ret = get_errno(safe_preadv(arg1, vec, arg3,
>>           pos,
>>           (pos >> HOST_HALF_LONG_BITS) >> HOST_HALF_LONG_BITS));
>>
>> It looks simpler, but perhaps I miss something
>> (it's just cut&paste, I don't pretend to understand that code...)?
> 
> If we decide that host unsigned long long is wide enough to represent
> meaningful file positions then this function may be simplified to
> something like
> 
> unsigned long long off = (unsigned long long)tlow |
> ((unsigned long long)thigh << (TARGET_LONG_BITS / 2)) << (TARGET_LONG_BITS / 
> 2);
> *hlow = (unsigned long)off;
> *hhigh = (unsigned long)((off >> (HOST_LONG_BITS / 2)) >> (HOST_LONG_BITS / 
> 2));
> 
> There's also a bug that the target may specify an offset not representable
> on the host, that will get truncated and point to a different location in the
> file, possibly resulting in data corruption.

I don't think it can happen, because "long long" is always 64bit, so it
fits in two 32bit (long is 32bit on 32bit, 64bit on 64bit).

Thanks,
Laurent