Anton Khirnov <[email protected]> writes:
>> + if ((chset->downmix_coeffs[i] = dca_get_dmix_coeff(s)) == -1)
>
> You're checking for -1, but the function returns a proper error code.
> Same below twice.
I don't think it's a good idea to have dca_get_dmix_coeff return a
proper error code in this way. The valid return values are a set of
positive and negative numbers. It's not obvious, at least not to me,
that this set doesn't intersect with the set of error codes. If
returning -1 on failure (a value carefully chosen to not collide with
the return value for any valid input) is considered ugly, I'd suggest
rewriting it as
static int dca_get_dmix_coeff(DCAContext *s, int32_t *coeff) { ... }
with return value following libav conventions, and the actual
coefficient value returned via the additional parameter. With some cost
in the conciseness at the call sites, which I'll happily leave to
other's judgement.
>> + return AVERROR_INVALIDDATA;
>> + } else {
>> + unsigned c, r;
>> + for (c = i = 0; c < s->xll_channels; c++, i += chset->channels +
>> 1) {
>> + if ((chset->downmix_coeffs[i] = dca_get_inv_dmix_coeff(s)) == -1)
>> + return AVERROR_INVALIDDATA;
>> + for (r = 1; r <= chset->channels; r++) {
>> + int32_t coeff = dca_get_dmix_coeff(s);
>> + if (coeff == -1)
>> + return AVERROR_INVALIDDATA;
>> + chset->downmix_coeffs[i + r] =
>> + (chset->downmix_coeffs[i] * (int64_t) coeff + (1 << 15)) >> 16;
>
> Maybe I'm just missing something, but seems to me this can overflow the
> array.
This code should be in a branch executed only if chset->downmix_ncoeffs
<= DCA_XLL_DMIX_NCOEFFS_MAX, which is an arbitrary limit. And the
intention is that chset->downmix_ncoeffs should equal the number of
coeffients read into the array.
In the case that primary_ch_set is false, downmix_ncoeffs is set to
(chset->channels + 1) * s->xll_channels, which I think matches the loop
logic.
It might make sense to add an assert (or whatever you prefer instead of
asserts) to check that the indices indeed are <
DCA_XLL_DMIX_NCOEFFS_MAX.
>> + data_start = get_bits_count(&s->gb);
>> + if (data_start + 8 * s->xll_navi.band_size[0] > asset_end) {
>> + av_log(s->avctx, AV_LOG_ERROR,
>> + "XLL: Data in NAVI table exceeds containing asset\n"
>> + "start: %d (bit), size %u (bytes), end %d (bit), error %u\n",
>> + data_start, s->xll_navi.band_size[0], asset_end,
>> + data_start + 8 * s->xll_navi.band_size[0] - asset_end);
>
> So shouldn't this return an error?
Yes, I think it should. Then, if I understand the code correctly, the
xll segment gets ignored, but the "core" data will be decoded.
Regards,
/Niels
--
Niels Möller. PGP-encrypted email is preferred. Keyid C0B98E26.
Internet email is subject to wholesale government surveillance.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
