On Sun, Mar 08, 2015 at 03:27:52PM +0100, Niels Möller wrote:
> Anton Khirnov <[email protected]> writes:
> >> + 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.
#define DCA_XLL_DMIX_NCOEFFS_MAX (18)
typedef struct XllChSetSubHeader {
int channels; ///< number of channels in channel set, at
most 16
int downmix_coeffs[DCA_XLL_DMIX_NCOEFFS_MAX];
This looks like it could overflow on the second iteration if channels is
16 as the comment claims it could be ...
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
