[alsa-devel] [PATCH] alsa-lib/tlv: fix handling of raw value ranges

[Takashi Iwai]

At Mon, 21 May 2012 21:38:22 +0200 (CEST),
Benoît Thébaudeau wrote:
> 
> Hi Takashi, Clemens, all,
> 
> Takashi Iwai wrote:
> > Benoît Thébaudeau wrote:
> > > 
> > > Clemens Ladisch wrote:
> > > > I'll see if I can cobble together some TLV checking code ...
> > > 
> > > Great.
> > > 
> > > > > Because it's useless to wait until the end of the loop to
> > > > > return
> > > > > -EINVAL,
> > > > 
> > > > It is not necessary to optimize for error cases.
> > > 
> > > It's not an optimization, but a simplification. Otherwise, pos
> > > would have to be
> > > tested again after the loop to differentiate the loop-skipped case
> > > from the
> > > at-least-one-loop-iteration cases to return either -EINVAL or the
> > > prev submax.
> > 
> > Unless it's really a hot path, such a micro-optimization doesn't give
> > much value.
> > 
> > Rather better to keep the standard style that makes people easier to
> > read the code.  A do-while loop with a large block is usually less
> > readable than a normal while loop.
> 
> Would you agree with the following revision?

Yes, the patch looks good to me.
But I couldn't apply it because the mail is encoded in
quoted-printable.  Could you fix it, or if it's difficult, give an
attachment?


thanks,

Takashi

> [PATCH] alsa-lib/tlv: improve robustness of raw value ranges
> 
> snd_tlv_convert_from_dB() relies on rangemin/max blindly. Since this function is
> exported, it is better for robustness and consistency to parse the range
> properly, which this patch does.
> 
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau at advansee.com>
> 
> --- alsa-lib/src/control/tlv.c
> +++ alsa-lib/src/control/tlv.c
> @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int
>  {
>          switch (tlv[0]) {
>          case SND_CTL_TLVT_DB_RANGE: {
> -                long dbmin, dbmax, prev_rangemax;
> +                long dbmin, dbmax, prev_submax;
>                  unsigned int pos, len;
>                  len = int_index(tlv[1]);
> -                if (len > MAX_TLV_RANGE_SIZE)
> -                        return -EINVAL;
> -                if (snd_tlv_get_dB_range(tlv, rangemin, rangemax,
> -                                         &dbmin, &dbmax))
> +                if (len < 6 || len > MAX_TLV_RANGE_SIZE)
>                          return -EINVAL;
> -                if (db_gain <= dbmin) {
> -                        *value = rangemin;
> -                        return 0;
> -                } else if (db_gain >= dbmax) {
> -                        *value = rangemax;
> -                        return 0;
> -                }
>                  pos = 2;
> -                prev_rangemax = 0;
> +                prev_submax = 0;
>                  while (pos + 4 <= len) {
> -                        rangemin = (int)tlv[pos];
> -                        rangemax = (int)tlv[pos + 1];
> +                        long submin, submax;
> +                        submin = (int)tlv[pos];
> +                        submax = (int)tlv[pos + 1];
> +                        if (rangemax < submax)
> +                                submax = rangemax;
>                          if (!snd_tlv_get_dB_range(tlv + pos + 2,
> -                                                  rangemin, rangemax,
> +                                                  submin, submax,
>                                                    &dbmin, &dbmax) &&
>                              db_gain >= dbmin && db_gain <= dbmax)
>                                  return snd_tlv_convert_from_dB(tlv + pos + 2,
> -                                                               rangemin, rangemax,
> +                                                               submin, submax,
>                                                                 db_gain, value, xdir);
>                          else if (db_gain < dbmin) {
> -                                *value = xdir ? rangemin : prev_rangemax;
> +                                *value = xdir || pos == 2 ? submin : prev_submax;
>                                  return 0;
>                          }
> -                        prev_rangemax = rangemax;
> +                        prev_submax = submax;
> +                        if (rangemax == submax)
> +                                break;
>                          pos += int_index(tlv[pos + 3]) + 4;
>                  }
> -                return -EINVAL;
> +                *value = prev_submax;
> +                return 0;
>          }
>          case SND_CTL_TLVT_DB_SCALE: {
>                  int min, step, max;
>