diff mbox series

[v2,52/52] usbaudio: change playback counters to 64 bit

Message ID 1415d73f73787a48532cc6cdd3c2a5a0c2e02e2f.1545598229.git.DirtY.iCE.hu@gmail.com (mailing list archive)
State New, archived
Headers show
Series Audio 5.1 patches | expand

Commit Message

Zoltán Kővágó Dec. 23, 2018, 8:52 p.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé Dec. 25, 2018, 10:50 a.m. UTC | #1 
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;
    }
Zoltán Kővágó Dec. 27, 2018, 10:08 p.m. UTC | #2 
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 mbox series

Patch

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) {