Hi,
On 2018-12-25 11:50, Philippe Mathieu-Daudé wrote:
> On 12/23/18 9:52 PM, Kővágó, Zoltán wrote:
>> With stereo playback, they need about 375 minutes of continuous audio
>> playback to overflow, which is usually not a problem (as stopping and
>> later resuming playback resets the counters). But with 7.1 audio, they
>> only need about 95 minutes to overflow.
>>
>> After the overflow, the buf->prod % USBAUDIO_PACKET_SIZE(channels)
>> assertion no longer holds true, which will result in overflowing the
>> buffer. With 64 bit variables, it would take about 762000 years to
>> overflow.
>>
>> Signed-off-by: Kővágó, Zoltán <dirty.ice...@gmail.com>
>> ---
>> hw/usb/dev-audio.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c
>> index 29475a2b70..45ffc3ebb3 100644
>> --- a/hw/usb/dev-audio.c
>> +++ b/hw/usb/dev-audio.c
>> @@ -577,9 +577,9 @@ static const USBDesc desc_audio_multi = {
>>
>> struct streambuf {
>> uint8_t *data;
>> - uint32_t size;
>> - uint32_t prod;
>> - uint32_t cons;
>> + size_t size;
>> + uint64_t prod;
>> + uint64_t cons;
>
> OK.
>
>> };
>>
>> static void streambuf_init(struct streambuf *buf, uint32_t size,
>> @@ -600,12 +600,14 @@ static void streambuf_fini(struct streambuf *buf)
>>
>> static int streambuf_put(struct streambuf *buf, USBPacket *p, uint32_t
>> channels)
>> {
>> - uint32_t free = buf->size - (buf->prod - buf->cons);
>> + uint64_t free = buf->size - (buf->prod - buf->cons);
>
> I'd use ssize_t here.
>
Hm, I agree with the signed part, but I'm not sure about the size_t
part. Granted, there's a big problem if buf->prod - buf->cons can't be
representated on 32 bits, but still I'd rather use int64_t in this case
and prevent this potential truncation on 32-bit platforms.
>>
>> if (free < USBAUDIO_PACKET_SIZE(channels)) {
>> return 0;
>> }
>>
>> + /* can happen if prod overflows */
>> + assert(buf->prod % USBAUDIO_PACKET_SIZE(channels) == 0);
>> usb_packet_copy(p, buf->data + (buf->prod % buf->size),
>> USBAUDIO_PACKET_SIZE(channels));
>> buf->prod += USBAUDIO_PACKET_SIZE(channels);
>> @@ -614,7 +616,7 @@ static int streambuf_put(struct streambuf *buf,
>> USBPacket *p, uint32_t channels)
>>
>> static uint8_t *streambuf_get(struct streambuf *buf, size_t *len)
>> {
>> - uint32_t used = buf->prod - buf->cons;
>> + uint64_t used = buf->prod - buf->cons;
>> uint8_t *data;
>>
>> if (!used) {
>
> Eventually here:
>
> ssize_t used = buf->prod - buf->cons;
>
> if (used <= 0) {
> return NULL;
> }
>
Regards,
Zoltan
