On 3/29/2022 4:02 PM, Michael Niedermayer wrote:
On Sat, Mar 19, 2022 at 06:52:34PM +0100, Michael Niedermayer wrote:
On Fri, Mar 18, 2022 at 08:38:28AM -0300, James Almer wrote:
On 3/18/2022 8:27 AM, Michael Niedermayer wrote:
On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote:
On 3/17/2022 9:07 PM, James Almer wrote:
On 3/17/2022 8:52 PM, James Almer wrote:
On 3/17/2022 8:30 PM, Michael Niedermayer wrote:
Fixes: Out of array write
Fixes:
45613/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-4539073606320128
Found-by: continuous fuzzing process
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc>
---
libavcodec/wmalosslessdec.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
index cd05b22689..1728920729 100644
--- a/libavcodec/wmalosslessdec.c
+++ b/libavcodec/wmalosslessdec.c
@@ -281,6 +281,9 @@ static av_cold int
decode_init(AVCodecContext *avctx)
av_channel_layout_uninit(&avctx->ch_layout);
av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
+ if (s->num_channels != avctx->ch_layout.nb_channels)
+ return AVERROR_PATCHWELCOME; //are there non fuzzed
files with this or is it an error ?
s->num_channels at this point is set to the channels count the user
set before calling avcodec_open2() (Normally from lavf), but it
could be anything.
If channel_mask is taken from extradata, maybe it should be used to
set s->num_channels instead of aborting because the user set value
and extradata disagreed.
Also, can you reproduce this crash before 3c933af493?
s->num_channels was being set to the user set channel count too,
same as now.
Right, before that commit s->num_channels and avctx->channels were
always the same, but avctx->channel_layout was whatever came from
extradata, and its popcnt could be != avctx->channels.
After it, avctx->ch_layout.nb_channels is always the same as
popcnt(avctx->ch_layout.u.mask), which can be different than
s->num_channels.
I think my suggestion above to use the extradata channel mask and
ignoring the user set channel count is the best approach for this.
Like this maybe (channel_mask could in theory be zero, so in that case the
user set value should be used).
diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
index cd05b22689..915add1962 100644
--- a/libavcodec/wmalosslessdec.c
+++ b/libavcodec/wmalosslessdec.c
@@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
return AVERROR_PATCHWELCOME;
}
- s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
- s->frame_data = av_mallocz(s->max_frame_size +
AV_INPUT_BUFFER_PADDING_SIZE);
- if (!s->frame_data)
- return AVERROR(ENOMEM);
-
- s->avctx = avctx;
- ff_llauddsp_init(&s->dsp);
- init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
-
if (avctx->extradata_size >= 18) {
s->decode_flags = AV_RL16(edata_ptr + 14);
channel_mask = AV_RL32(edata_ptr + 2);
@@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
return AVERROR_PATCHWELCOME;
}
+ if (channel_mask) {
+ av_channel_layout_uninit(&avctx->ch_layout);
+ av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
+ } else
+ avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
+
+ s->num_channels = avctx->ch_layout.nb_channels;
+
+ /* extract lfe channel position */
+ s->lfe_channel = -1;
+
+ if (channel_mask & 8) {
+ unsigned int mask;
+ for (mask = 1; mask < 16; mask <<= 1)
+ if (channel_mask & mask)
+ ++s->lfe_channel;
+ }
+
+ s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
+ s->frame_data = av_mallocz(s->max_frame_size +
AV_INPUT_BUFFER_PADDING_SIZE);
+ if (!s->frame_data)
+ return AVERROR(ENOMEM);
+
+ s->avctx = avctx;
+ ff_llauddsp_init(&s->dsp);
+ init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
+
/* generic init */
s->log2_frame_size = av_log2(avctx->block_align) + 4;
@@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
return AVERROR_INVALIDDATA;
}
- s->num_channels = avctx->ch_layout.nb_channels;
-
- /* extract lfe channel position */
- s->lfe_channel = -1;
-
- if (channel_mask & 8) {
- unsigned int mask;
- for (mask = 1; mask < 16; mask <<= 1)
- if (channel_mask & mask)
- ++s->lfe_channel;
- }
-
s->frame = av_frame_alloc();
if (!s->frame)
return AVERROR(ENOMEM);
- av_channel_layout_uninit(&avctx->ch_layout);
- av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
return 0;
}
this will change the output of the "lossless" decoder if this case ever
occurs besides this
As i interpret it, you could initialize the decoder using any arbitrary
number of channels in avctx. And if channel_mask in extradata is not zero,
then it must be considered to be the valid channel count for the stream, and
the value passed in avctx should be ignored.
without a specification and no non fuzzed testfile its really not possible
to say for sure what this combination is intended to produce on the output
What you say sounds reasonable but it can also be something else
ping, i think your patch or something similar should be applied
The fuzzer found
46008/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-4681245747970048
which too crashes because of the channel number
thx
Applied.
_______________________________________________
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".
