Message ID | 1415d73f73787a48532cc6cdd3c2a5a0c2e02e2f.1545598229.git.DirtY.iCE.hu@gmail.com |
---|---|
State | New |
Headers | show |
Series | Audio 5.1 patches | expand |
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.hu@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. > > 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; }
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.hu@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
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; }; 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); 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) {
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.hu@gmail.com> --- hw/usb/dev-audio.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)