| Message ID | 1396605482-8720-8-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
* arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote: > From: ChenLiang <chenliang88@huawei.com> > > xbzrle_encode_buffer checks the value in the vm ram repeatedly. > It is risk if runs xbzrle_encode_buffer on changing data. > And it is not necessary. > > Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: ChenLiang <chenliang88@huawei.com> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > xbzrle.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/xbzrle.c b/xbzrle.c > index fbcb35d..92cccd7 100644 > --- a/xbzrle.c > +++ b/xbzrle.c > @@ -27,9 +27,10 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, > uint8_t *dst, int dlen) > { > uint32_t zrun_len = 0, nzrun_len = 0; > - int d = 0, i = 0; > + int d = 0, i = 0, j; > long res, xor; > uint8_t *nzrun_start = NULL; > + uint8_t *xor_ptr = (uint8_t *)(&xor); > > g_assert(!(((uintptr_t)old_buf | (uintptr_t)new_buf | slen) % > sizeof(long))); > @@ -82,6 +83,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, > if (d + 2 > dlen) { > return -1; > } > + i++; > + nzrun_len++; Yes, I think that's safe - I was checking for if an overflow was possible, but my reading is that before this 'i++' i can be a maximum of slen-1, so here it's a maximum of slen and the next loop won't happen in that case. > /* not aligned to sizeof(long) */ > res = (slen - i) % sizeof(long); > while (res && old_buf[i] != new_buf[i]) { > @@ -98,11 +101,16 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, > xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i); > if ((xor - mask) & ~xor & (mask << 7)) { > /* found the end of an nzrun within the current long */ > - while (old_buf[i] != new_buf[i]) { > - nzrun_len++; > - i++; > + for (j = 0; j < sizeof(long); j++) { > + if (0 == xor_ptr[j]) { > + break; > + } > + } > + i += j; > + nzrun_len += j; > + if (j != sizeof(long)) { > + break; > } > - break; > } else { > i += sizeof(long); > nzrun_len += sizeof(long); > @@ -118,6 +126,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, > memcpy(dst + d, nzrun_start, nzrun_len); > d += nzrun_len; > nzrun_len = 0; > + i++; > + zrun_len++; I think that's also safe, because if i was now 'slen' the mainloop would exit, that would mean the last zero run wasn't encoded, but there seems to already be a check that causes the last zero run not to be encoded. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > } > > return d; > -- > 1.7.12.4 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/xbzrle.c b/xbzrle.c index fbcb35d..92cccd7 100644 --- a/xbzrle.c +++ b/xbzrle.c @@ -27,9 +27,10 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, uint8_t *dst, int dlen) { uint32_t zrun_len = 0, nzrun_len = 0; - int d = 0, i = 0; + int d = 0, i = 0, j; long res, xor; uint8_t *nzrun_start = NULL; + uint8_t *xor_ptr = (uint8_t *)(&xor); g_assert(!(((uintptr_t)old_buf | (uintptr_t)new_buf | slen) % sizeof(long))); @@ -82,6 +83,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, if (d + 2 > dlen) { return -1; } + i++; + nzrun_len++; /* not aligned to sizeof(long) */ res = (slen - i) % sizeof(long); while (res && old_buf[i] != new_buf[i]) { @@ -98,11 +101,16 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i); if ((xor - mask) & ~xor & (mask << 7)) { /* found the end of an nzrun within the current long */ - while (old_buf[i] != new_buf[i]) { - nzrun_len++; - i++; + for (j = 0; j < sizeof(long); j++) { + if (0 == xor_ptr[j]) { + break; + } + } + i += j; + nzrun_len += j; + if (j != sizeof(long)) { + break; } - break; } else { i += sizeof(long); nzrun_len += sizeof(long); @@ -118,6 +126,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, memcpy(dst + d, nzrun_start, nzrun_len); d += nzrun_len; nzrun_len = 0; + i++; + zrun_len++; } return d;