Re: [Qemu-devel] [PATCH] xbzrle: don't check the value in the vm ram repeatedly

[Eric Blake]

On 03/29/2014 01:52 AM, address@hidden wrote:
> From: ChenLiang <address@hidden>
> 
> xbzrle_encode_buffer checks the value in the ram repeatedly.
> It is risk if runs xbzrle_encode_buffer on changing data.
> And it is not necessary.
> 
> Reported-by: Dr. David Alan Gilbert <address@hidden>
> Signed-off-by: ChenLiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  xbzrle.c | 4 ++++
>  1 file changed, 4 insertions(+)

Insufficient.  Observe what we did at line 52 when looking for the zero-run:

>>         /* word at a time for speed */
>>         if (!res) {
>>             while (i < slen &&
>>                    (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {

This dereferences 8 bytes at new_buf[i] (on a 64-bit machine)...

>>                 i += sizeof(long);
>>                 zrun_len += sizeof(long);
>>             }
>> 
>>             /* go over the rest */
>>             while (i < slen && old_buf[i] == new_buf[i]) {

...and this also dereferences those same bytes.  But your argument is
that new_buf[i] is volatile.

You MUST read new_buf[i] into a temporary variable, and if the first
read found a difference, then the "go over the rest" analysis must be on
that temporary, rather than going back to reading directly from new_buf.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature