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. > diff --git a/sbc/sbc.c b/sbc/sbc.c > index 606f11c..ee59086 100644 > --- a/sbc/sbc.c > +++ b/sbc/sbc.c > @@ -1222,6 +1222,15 @@ 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 (!output) > + return framelen; > + > + if (written) > + *written = 0; > + > + if (framelen <= 0) > + return framelen; > + > if (!priv->init) { > sbc_decoder_init(&priv->dec_state, &priv->frame); > priv->init = true; > @@ -1240,15 +1249,6 @@ SBC_EXPORT ssize_t sbc_decode(sbc_t *sbc, const > void *input, size_t input_len, > sbc->bitpool = priv->frame.bitpool; > } > > - if (!output) > - return framelen; > - > - if (written) > - *written = 0; > - > - if (framelen <= 0) > - return framelen; > - > samples = sbc_synthesize_audio(&priv->dec_state, &priv->frame); > > ptr = output; > > -- > 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
