On 4/5/20 1:32 PM, Anton Khirnov wrote: > Fix invalid memory access on DSD streams with changing channel count. > --- > libavcodec/wavpack.c | 122 +++++++++++++++++++++++++++++++------------ > 1 file changed, 90 insertions(+), 32 deletions(-) > > diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c > index b27262b94e..9cc4104dd0 100644 > --- a/libavcodec/wavpack.c > +++ b/libavcodec/wavpack.c > @@ -20,6 +20,7 @@ > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 > USA > */ > > +#include "libavutil/buffer.h" > #include "libavutil/channel_layout.h" > > #define BITSTREAM_READER_LE > @@ -109,7 +110,10 @@ typedef struct WavpackContext { > AVFrame *frame; > ThreadFrame curr_frame, prev_frame; > Modulation modulation; > + > + AVBufferRef *dsd_ref; > DSDContext *dsdctx; > + int dsd_channels; > } WavpackContext; > > #define LEVEL_DECAY(a) (((a) + 0x80) >> 8) > @@ -978,6 +982,32 @@ static av_cold int wv_alloc_frame_context(WavpackContext > *c) > return 0; > } > > +static int wv_dsd_reset(WavpackContext *s, int channels) > +{ > + int i; > + > + s->dsdctx = NULL; > + s->dsd_channels = 0; > + av_buffer_unref(&s->dsd_ref); > + > + if (!channels) > + return 0; > + > + if (channels > INT_MAX / sizeof(*s->dsdctx)) > + return AVERROR(EINVAL); > + > + s->dsd_ref = av_buffer_allocz(channels * sizeof(*s->dsdctx)); > + if (!s->dsd_ref) > + return AVERROR(ENOMEM); > + s->dsdctx = (DSDContext*)s->dsd_ref->data; > + s->dsd_channels = channels; > + > + for (i = 0; i < channels; i++) > + memset(s->dsdctx[i].buf, 0x69, sizeof(s->dsdctx[i].buf)); > + > + return 0; > +} > + > #if HAVE_THREADS > static int init_thread_copy(AVCodecContext *avctx) > { > @@ -1008,6 +1038,17 @@ static int update_thread_context(AVCodecContext *dst, > const AVCodecContext *src) > return ret; > } > > + av_buffer_unref(&fdst->dsd_ref); > + fdst->dsdctx = NULL; > + fdst->dsd_channels = 0; > + if (fsrc->dsd_ref) { > + fdst->dsd_ref = av_buffer_ref(fsrc->dsd_ref); > + if (!fdst->dsd_ref) > + return AVERROR(ENOMEM); > + fdst->dsdctx = (DSDContext*)fdst->dsd_ref->data; > + fdst->dsd_channels = fsrc->dsd_channels; > + } > + > return 0; > } > #endif > @@ -1025,15 +1066,9 @@ static av_cold int wavpack_decode_init(AVCodecContext > *avctx) > s->curr_frame.f = av_frame_alloc(); > s->prev_frame.f = av_frame_alloc(); > > - // the DSD to PCM context is shared (and used serially) between all > decoding threads > - s->dsdctx = av_calloc(avctx->channels, sizeof(DSDContext)); > - > - if (!s->curr_frame.f || !s->prev_frame.f || !s->dsdctx) > + if (!s->curr_frame.f || !s->prev_frame.f) > return AVERROR(ENOMEM); > > - for (int i = 0; i < avctx->channels; i++) > - memset(s->dsdctx[i].buf, 0x69, sizeof(s->dsdctx[i].buf)); > - > ff_init_dsd_data(); > > return 0; > @@ -1053,8 +1088,7 @@ static av_cold int wavpack_decode_end(AVCodecContext > *avctx) > ff_thread_release_buffer(avctx, &s->prev_frame); > av_frame_free(&s->prev_frame.f); > > - if (!avctx->internal->is_copy) > - av_freep(&s->dsdctx); > + av_buffer_unref(&s->dsd_ref); > > return 0; > } > @@ -1065,6 +1099,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, > int block_no, > WavpackContext *wc = avctx->priv_data; > WavpackFrameContext *s; > GetByteContext gb; > + enum AVSampleFormat sample_fmt; > void *samples_l = NULL, *samples_r = NULL; > int ret; > int got_terms = 0, got_weights = 0, got_samples = 0, > @@ -1102,7 +1137,15 @@ static int wavpack_decode_block(AVCodecContext *avctx, > int block_no, > return AVERROR_INVALIDDATA; > } > s->frame_flags = bytestream2_get_le32(&gb); > - bpp = av_get_bytes_per_sample(avctx->sample_fmt); > + > + if (s->frame_flags & (WV_FLOAT_DATA | WV_DSD_DATA)) > + sample_fmt = AV_SAMPLE_FMT_FLTP; > + else if ((s->frame_flags & 0x03) <= 1) > + sample_fmt = AV_SAMPLE_FMT_S16P; > + else > + sample_fmt = AV_SAMPLE_FMT_S32P; > + > + bpp = av_get_bytes_per_sample(sample_fmt); > orig_bpp = ((s->frame_flags & 0x03) + 1) << 3; > multiblock = (s->frame_flags & WV_SINGLE_BLOCK) != WV_SINGLE_BLOCK; > > @@ -1436,11 +1479,11 @@ static int wavpack_decode_block(AVCodecContext > *avctx, int block_no, > av_log(avctx, AV_LOG_ERROR, "Hybrid config not found\n"); > return AVERROR_INVALIDDATA; > } > - if (!got_float && avctx->sample_fmt == AV_SAMPLE_FMT_FLTP) { > + if (!got_float && sample_fmt == AV_SAMPLE_FMT_FLTP) { > av_log(avctx, AV_LOG_ERROR, "Float information not found\n"); > return AVERROR_INVALIDDATA; > } > - if (s->got_extra_bits && avctx->sample_fmt != AV_SAMPLE_FMT_FLTP) { > + if (s->got_extra_bits && sample_fmt != AV_SAMPLE_FMT_FLTP) { > const int size = get_bits_left(&s->gb_extra_bits); > const int wanted = s->samples * s->extra_bits << s->stereo_in; > if (size < wanted) { > @@ -1462,27 +1505,54 @@ static int wavpack_decode_block(AVCodecContext > *avctx, int block_no, > } > > if (!wc->ch_offset) { > + int new_channels = avctx->channels; > + uint64_t new_chmask = avctx->channel_layout; > + int new_samplerate; > int sr = (s->frame_flags >> 23) & 0xf; > if (sr == 0xf) { > if (!sample_rate) { > av_log(avctx, AV_LOG_ERROR, "Custom sample rate missing.\n"); > return AVERROR_INVALIDDATA; > } > - avctx->sample_rate = sample_rate * rate_x; > + new_samplerate = sample_rate * rate_x; > } else > - avctx->sample_rate = wv_rates[sr] * rate_x; > + new_samplerate = wv_rates[sr] * rate_x; > > if (multiblock) { > if (chan) > - avctx->channels = chan; > + new_channels = chan; > if (chmask) > - avctx->channel_layout = chmask; > + new_chmask = chmask; > } else { > - avctx->channels = s->stereo ? 2 : 1; > - avctx->channel_layout = s->stereo ? AV_CH_LAYOUT_STEREO : > - AV_CH_LAYOUT_MONO; > + new_channels = s->stereo ? 2 : 1; > + new_chmask = s->stereo ? AV_CH_LAYOUT_STEREO : > + AV_CH_LAYOUT_MONO; > + } > + > + if (new_chmask && > + av_get_channel_layout_nb_channels(new_chmask) != new_channels) { > + av_log(avctx, AV_LOG_ERROR, "Channel mask does not match the > channel count\n"); > + return AVERROR_INVALIDDATA; > } > > + /* clear DSD state if stream properties change */ > + if (new_channels != wc->dsd_channels || > + new_chmask != avctx->channel_layout || > + new_samplerate != avctx->sample_rate || > + !!got_dsd != !!wc->dsdctx) { > + ret = wv_dsd_reset(wc, got_dsd ? new_channels : 0); > + if (ret < 0) { > + av_log(avctx, AV_LOG_ERROR, "Error reinitializing the DSD > context\n"); > + return ret; > + } > + ff_thread_release_buffer(avctx, &wc->curr_frame); > + } > + avctx->channels = new_channels; > + avctx->channel_layout = new_chmask; > + avctx->sample_rate = new_samplerate; > + avctx->sample_fmt = sample_fmt; > + avctx->bits_per_raw_sample = orig_bpp; > + > ff_thread_release_buffer(avctx, &wc->prev_frame); > FFSWAP(ThreadFrame, wc->curr_frame, wc->prev_frame); > > @@ -1546,10 +1616,7 @@ static void wavpack_decode_flush(AVCodecContext *avctx) > { > WavpackContext *s = avctx->priv_data; > > - if (!avctx->internal->is_copy) { > - for (int i = 0; i < avctx->channels; i++) > - memset(s->dsdctx[i].buf, 0x69, sizeof(s->dsdctx[i].buf)); > - } > + wv_dsd_reset(s, 0); > } > > static int dsd_channel(AVCodecContext *avctx, void *frmptr, int jobnr, int > threadnr) > @@ -1590,15 +1657,6 @@ static int wavpack_decode_frame(AVCodecContext *avctx, > void *data, > > s->modulation = (frame_flags & WV_DSD_DATA) ? MODULATION_DSD : > MODULATION_PCM; > > - if (frame_flags & (WV_FLOAT_DATA | WV_DSD_DATA)) { > - avctx->sample_fmt = AV_SAMPLE_FMT_FLTP; > - } else if ((frame_flags & 0x03) <= 1) { > - avctx->sample_fmt = AV_SAMPLE_FMT_S16P; > - } else { > - avctx->sample_fmt = AV_SAMPLE_FMT_S32P; > - avctx->bits_per_raw_sample = ((frame_flags & 0x03) + 1) << 3; > - } > - > while (buf_size > WV_HEADER_SIZE) { > frame_size = AV_RL32(buf + 4) - 12; > buf += 20;
I was working on implementing this myself, but this is a better solution. I did not use AVBufferRef, but achieved a similar thing with multiple pointers back to the initial context (one for the DSD context and one for the DSD channel count). I have tested this with my reference files, and also a new one that artificially changes the channel configuration in DSD mode. That crashed the previous version (even without Matroska) and now works fine. Good job! _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".