2015-12-15 20:54 GMT+02:00 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>: > Hi Maxim, > > On Tue, Dec 15, 2015 at 4:27 PM, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote: >> 2015-12-15 20:04 GMT+02:00 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>: >>> Hi Maxim, >>> >>> On Tue, Dec 15, 2015 at 3:27 PM, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote: >>>> Hi Luiz, >>>> >>>> 2015-12-15 18:51 GMT+02:00 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>: >>>>> Hi Maxim, >>>>> >>>>> On Mon, Dec 14, 2015 at 3:27 PM, <maxtram95@xxxxxxxxx> wrote: >>>>>> From: Maxim Mikityanskiy <maxtram95@xxxxxxxxx> >>>>>> >>>>>> If the first data chunk passed to sbc_decode was not long enough and >>>>>> didn't contain full SBC packet, don't try to initialize codec parameters >>>>>> with random values. >>>>>> >>>>>> Signed-off-by: Maxim Mikityanskiy <maxtram95@xxxxxxxxx> >>>>>> Cc: Marcel Holtmann <marcel@xxxxxxxxxxxx> >>>>>> --- >>>>>> The patch is for SBC library located at https://git.kernel.org/cgit/bluetooth/sbc.git/ >>>>> >>>>> We don't use Signed-off-by in userspace. >>>> >>>> OK, I'll consider it. >>>> >>>>>> >>>>>> sbc/sbc.c | 34 ++++++++++++++++++---------------- >>>>>> 1 file changed, 18 insertions(+), 16 deletions(-) >>>>>> >>>>>> diff --git a/sbc/sbc.c b/sbc/sbc.c >>>>>> index 606f11c..7da476b 100644 >>>>>> --- a/sbc/sbc.c >>>>>> +++ b/sbc/sbc.c >>>>>> @@ -1222,22 +1222,24 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len, >>>>>> >>>>>> framelen = priv->unpack_frame(input, &priv->frame, input_len); >>>>>> >>>>>> - if (!priv->init) { >>>>>> - sbc_decoder_init(&priv->dec_state, &priv->frame); >>>>>> - priv->init = true; >>>>>> - >>>>>> - sbc->frequency = priv->frame.frequency; >>>>>> - sbc->mode = priv->frame.mode; >>>>>> - sbc->subbands = priv->frame.subband_mode; >>>>>> - sbc->blocks = priv->frame.block_mode; >>>>>> - sbc->allocation = priv->frame.allocation; >>>>>> - sbc->bitpool = priv->frame.bitpool; >>>>>> - >>>>>> - priv->frame.codesize = sbc_get_codesize(sbc); >>>>>> - priv->frame.length = framelen; >>>>>> - } else if (priv->frame.bitpool != sbc->bitpool) { >>>>>> - priv->frame.length = framelen; >>>>>> - sbc->bitpool = priv->frame.bitpool; >>>>>> + if (framelen >= 0) { >>>>>> + if (!priv->init) { >>>>>> + sbc_decoder_init(&priv->dec_state, &priv->frame); >>>>>> + priv->init = true; >>>>>> + >>>>>> + sbc->frequency = priv->frame.frequency; >>>>>> + sbc->mode = priv->frame.mode; >>>>>> + sbc->subbands = priv->frame.subband_mode; >>>>>> + sbc->blocks = priv->frame.block_mode; >>>>>> + sbc->allocation = priv->frame.allocation; >>>>>> + sbc->bitpool = priv->frame.bitpool; >>>>>> + >>>>>> + priv->frame.codesize = sbc_get_codesize(sbc); >>>>>> + priv->frame.length = framelen; >>>>>> + } else if (priv->frame.bitpool != sbc->bitpool) { >>>>>> + priv->frame.length = framelen; >>>>>> + sbc->bitpool = priv->frame.bitpool; >>>>>> + } >>>>> >>>>> Im fine with the change but I would prefix that you check the framelen >>>>> separately e.g.: >>>>> >>>>> if (framelen < 0) { >>>>> return framelen; >>>>> } >>>>> >>>>> There is probably no use to continue if unpack_frame has failed. >>>> >>>> Actually, we can't just return immediately, because we will lose an >>>> assignment "*written = 0;" that follows this block of code. And >>>> putting that assignment also in "if (framelen < 0)" will lead to code >>>> duplication. We could place that assignment before the check for >>>> framelen, but it will change the behavior when output == NULL (now >>>> *written is not touched if output == NULL). >>> >>> And with the changed you are proposing the code will evaluate framelen >>> twice so how about the following: >> >> That has even worse problems, because if output == NULL (the caller >> doesn't want the result to be saved) and framelen > 0, we'll just >> return immediately and skip initialization of decoder. > > Yep, perhaps it would make more sense to have sbc_decode call > sbc_parse and not the other way around that way we can actually return > an error if output is NULL: I agree, the code really looks better with this change. However, there may be projects that call sbc_decode with output == NULL directly instead of using sbc_parse, so that projects will break after this change. Don't know if we should worry about them (even I don't know if such projects exist). Anyway, if we want to maintain compatibility with them, we could check "if (!output) return framelen;" after calling sbc_parse. But the decision is up to you. > diff --git a/sbc/sbc.c b/sbc/sbc.c > index 606f11c..5af900d 100644 > --- a/sbc/sbc.c > +++ b/sbc/sbc.c > @@ -1205,15 +1205,8 @@ int sbc_reinit_a2dp(sbc_t *sbc, unsigned long flags, > > SBC_EXPORT ssize_t sbc_parse(sbc_t *sbc, const void *input, size_t input_len) > { > - return sbc_decode(sbc, input, input_len, NULL, 0, NULL); > -} > - > -SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len, > - void *output, size_t output_len, size_t *written) > -{ > struct sbc_priv *priv; > - char *ptr; > - int i, ch, framelen, samples; > + int framelen; > > if (!sbc || !input) > return -EIO; > @@ -1222,6 +1215,9 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const > void *input, size_t input_len, > > framelen = priv->unpack_frame(input, &priv->frame, input_len); > > + if (framelen <= 0) > + return framelen; > + > if (!priv->init) { > sbc_decoder_init(&priv->dec_state, &priv->frame); > priv->init = true; > @@ -1240,8 +1236,20 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const > void *input, size_t input_len, > sbc->bitpool = priv->frame.bitpool; > } > > + return framelen; > +} > + > +SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len, > + void *output, size_t output_len, size_t *written) > +{ > + struct sbc_priv *priv; > + char *ptr; > + int i, ch, framelen, samples; > + > if (!output) > - return framelen; > + return -EINVAL; > + > + framelen = sbc_parse(sbc, input, input_len); > > if (written) > *written = 0; > > > Luiz Augusto von Dentz -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
