Message ID | 4D709619.5020103@msgid.tls.msk.ru |
---|---|
State | New |
Headers | show |
>> >> bytes = zstream->total_out - previous_out; Good catch > total_out isn't used by zlib internally, so if the resulting > "total" counter is not needed in qemu, we can just zero-out > the total_out in this function before calling zlib, and > use the resulting value directly as "bytes", without > saving its previous value in previous_out. Something like > the attached patch does. If you're certain that total_out is not used by zlib, could you also send a patch for zlib encoding please ? (vnc-enc-zlib.c) Thanks,
On Thu, Mar 3, 2011 at 11:34 PM, Michael Tokarev <mjt@tls.msk.ru> wrote: >> The fix for this is simple: keep previous_out as a uLong too, which >> avoids any problems with sign conversion or truncation. > > This looks wrong to me. On 32bit x86 uLong is 32bits. Yes > it's unsigned there, but it's still 32bits. And sure thing > it _will_ overflow again, not at 2Gb but now at 4Gb, and the > next total_out will start from zero again, so that now bytes > will be a large integer again because "next" (new total_out) > will be less than "current" (previous_out). Actually there is no problem with overflow of unsigned long. The C standard says that unsigned arithmetic is simply done modulo the size of the integer, so when total_out reaches 4GB, things will just wrap around (and the difference between "nearby" values will still be the correct, small value). For example, if previous were (4GB - 5) and then total_out had 1000 added to it, total_out would end up as 995, and total_out - previous would be 1000. In other words I'm pretty sure my fix works on 32 bits too. However your fix using avail_out is probably better anyway, and your latest patch fixes both instances of the bug, so we might as well go with that. Thanks, - R.
On Fri, Mar 4, 2011 at 8:59 AM, Roland Dreier <roland@kernel.org> wrote: > Actually there is no problem with overflow of unsigned long. > The C standard says that unsigned arithmetic is simply done > modulo the size of the integer, so when total_out reaches > 4GB, things will just wrap around (and the difference > between "nearby" values will still be the correct, small > value). For example, if previous were (4GB - 5) and > then total_out had 1000 added to it, total_out would > end up as 995, and total_out - previous would be 1000. Additionally, thinking about this further, I realize that amusingly enough, the old code also works on 32-bit: the bug occurred because when we put a value above 2GB in a (32-bit) int, it became a signed quantity, which then became a gigantic value when promoted back to an unsigned (64-bit) long, which causes the subtraction to get the wrong value. On 32-bit, the promotion from signed 32-bit to unsigned 32-bit doesn't lead to the wrong difference. - R.
fix overflow after 2Gb of output in ui/vnc-enc-tight.c When amount of compressed data is more than 2Gb we will hit integer overflow (or when it's unsigned, the border is 4Gb). We don't use zstream->total_out for anything, so instead of remembering its previois value and compare with hext one, we can just reset it on entry and use its resulting value as the amount of bytes to deal with. diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c index 2522936..e1843cb 100644 --- a/ui/vnc-enc-tight.c +++ b/ui/vnc-enc-tight.c @@ -849,7 +849,6 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes, int level, int strategy) { z_streamp zstream = &vs->tight.stream[stream_id]; - int previous_out; if (bytes < VNC_TIGHT_MIN_TO_COMPRESS) { vnc_write(vs, vs->tight.tight.buffer, vs->tight.tight.offset); @@ -869,7 +868,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes, zstream->next_out = vs->tight.zlib.buffer + vs->tight.zlib.offset; zstream->avail_out = vs->tight.zlib.capacity - vs->tight.zlib.offset; zstream->data_type = Z_BINARY; - previous_out = zstream->total_out; + zstream->total_out = 0; /* start encoding */ if (deflate(zstream, Z_SYNC_FLUSH) != Z_OK) { @@ -878,7 +877,7 @@ static int tight_compress_data(VncState *vs, int stream_id, size_t bytes, } vs->tight.zlib.offset = vs->tight.zlib.capacity - zstream->avail_out; - bytes = zstream->total_out - previous_out; + bytes = zstream->total_out; tight_send_compact_size(vs, bytes); vnc_write(vs, vs->tight.zlib.buffer, bytes);