Romain Beauxis:
> Le ven. 30 mai 2025 à 19:44, Andreas Rheinhardt
> <andreas.rheinha...@outlook.com> a écrit :
>>
>> Romain Beauxis:
>>> ---
>>> libavcodec/vorbis_parser.h | 11 ++++
>>> libavcodec/vorbisdec.c | 75 +++++++++++++---------
>>> libavformat/oggparsevorbis.c | 67 ++++++++++++++++++-
>>> tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
>>> tests/ref/fate/trac-2739.txt | 4 +-
>>> 5 files changed, 121 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
>>> index 789932ac49..b176fe536c 100644
>>> --- a/libavcodec/vorbis_parser.h
>>> +++ b/libavcodec/vorbis_parser.h
>>> @@ -30,6 +30,17 @@
>>>
>>> typedef struct AVVorbisParseContext AVVorbisParseContext;
>>>
>>> +/**
>>> + * Used by the vorbis parser to pass new chained stream headers
>>> + * as extradata.
>>> + */
>>> +typedef struct vorbis_new_extradata {
>>> + uint8_t *header;
>>> + size_t header_size;
>>> + uint8_t *setup;
>>> + size_t setup_size;
>>> +} vorbis_new_extradata;
>>> +
>>> /**
>>> * Allocate and initialize the Vorbis parser using headers in the
>>> extradata.
>>> */
>>> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
>>> index adbd726183..a4b159ba9b 100644
>>> --- a/libavcodec/vorbisdec.c
>>> +++ b/libavcodec/vorbisdec.c
>>> @@ -43,6 +43,7 @@
>>> #include "vorbis.h"
>>> #include "vorbisdsp.h"
>>> #include "vorbis_data.h"
>>> +#include "vorbis_parser.h"
>>> #include "xiph.h"
>>>
>>> #define V_NB_BITS 8
>>> @@ -1778,47 +1779,59 @@ static int vorbis_decode_frame(AVCodecContext
>>> *avctx, AVFrame *frame,
>>> GetBitContext *gb = &vc->gb;
>>> float *channel_ptrs[255];
>>> int i, len, ret;
>>> + size_t new_extradata_size;
>>> + vorbis_new_extradata *new_extradata;
>>> + const uint8_t *header;
>>> + const uint8_t *setup;
>>>
>>> ff_dlog(NULL, "packet length %d \n", buf_size);
>>>
>>> - if (*buf == 1 && buf_size > 7) {
>>> - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
>>> - return ret;
>>> + new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
>>> + avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
>>>
>>> - vorbis_free(vc);
>>> - if ((ret = vorbis_parse_id_hdr(vc))) {
>>> - av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
>>> - vorbis_free(vc);
>>> - return ret;
>>> - }
>>> + if (new_extradata) {
>>> + header = new_extradata->header;
>>> + setup = new_extradata->setup;
>>>
>>> - av_channel_layout_uninit(&avctx->ch_layout);
>>> - if (vc->audio_channels > 8) {
>>> - avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
>>> - avctx->ch_layout.nb_channels = vc->audio_channels;
>>> - } else {
>>> - av_channel_layout_copy(&avctx->ch_layout,
>>> &ff_vorbis_ch_layouts[vc->audio_channels - 1]);
>>> - }
>>> + if (new_extradata->header_size > 7 && *header == 1) {
>>> + if ((ret = init_get_bits8(
>>> + gb, header + 1,
>>> + new_extradata->header_size - 1)) < 0)
>>> + return ret;
>>>
>>> - avctx->sample_rate = vc->audio_samplerate;
>>> - return buf_size;
>>> - }
>>> + vorbis_free(vc);
>>> + if ((ret = vorbis_parse_id_hdr(vc))) {
>>> + av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
>>> + vorbis_free(vc);
>>> + return ret;
>>> + }
>>>
>>> - if (*buf == 3 && buf_size > 7) {
>>> - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
>>> - return buf_size;
>>> - }
>>> + av_channel_layout_uninit(&avctx->ch_layout);
>>> + if (vc->audio_channels > 8) {
>>> + avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
>>> + avctx->ch_layout.nb_channels = vc->audio_channels;
>>> + } else {
>>> + av_channel_layout_copy(
>>> + &avctx->ch_layout,
>>> + &ff_vorbis_ch_layouts[vc->audio_channels - 1]);
>>> + }
>>>
>>> - if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
>>> - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
>>> - return ret;
>>> + avctx->sample_rate = vc->audio_samplerate;
>>> + }
>>>
>>> - if ((ret = vorbis_parse_setup_hdr(vc))) {
>>> - av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
>>> - vorbis_free(vc);
>>> - return ret;
>>> + if (new_extradata->setup_size > 7 && *setup == 5 &&
>>> + vc->channel_residues && !vc->modes) {
>>> + if ((ret = init_get_bits8(
>>> + gb, setup + 1,
>>> + new_extradata->setup_size - 1)) < 0)
>>> + return ret;
>>> +
>>> + if ((ret = vorbis_parse_setup_hdr(vc))) {
>>> + av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
>>> + vorbis_free(vc);
>>> + return ret;
>>> + }
>>> }
>>> - return buf_size;
>>> }
>>>
>>> if (!vc->channel_residues || !vc->modes) {
>>> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
>>> index 62cc2da6de..f8e66e8127 100644
>>> --- a/libavformat/oggparsevorbis.c
>>> +++ b/libavformat/oggparsevorbis.c
>>> @@ -255,12 +255,19 @@ static void vorbis_cleanup(AVFormatContext *s, int
>>> idx)
>>> struct ogg *ogg = s->priv_data;
>>> struct ogg_stream *os = ogg->streams + idx;
>>> struct oggvorbis_private *priv = os->private;
>>> + vorbis_new_extradata *new_extradata;
>>> int i;
>>> if (os->private) {
>>> av_vorbis_parse_free(&priv->vp);
>>> for (i = 0; i < 3; i++)
>>> av_freep(&priv->packet[i]);
>>> }
>>> +
>>> + if (os->new_extradata) {
>>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
>>> + av_freep(&new_extradata->header);
>>> + av_freep(&new_extradata->setup);
>>> + }
>>> }
>>>
>>> static int vorbis_update_metadata(AVFormatContext *s, int idx)
>>> @@ -433,7 +440,10 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>>> struct ogg *ogg = s->priv_data;
>>> struct ogg_stream *os = ogg->streams + idx;
>>> struct oggvorbis_private *priv = os->private;
>>> + vorbis_new_extradata *new_extradata;
>>> int duration, flags = 0;
>>> + int skip_packet = 0;
>>> + int ret;
>>>
>>> if (!priv->vp)
>>> return AVERROR_INVALIDDATA;
>>> @@ -496,10 +506,61 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>>> if (duration < 0) {
>>> os->pflags |= AV_PKT_FLAG_CORRUPT;
>>> return 0;
>>> - } else if (flags & VORBIS_FLAG_COMMENT) {
>>> - vorbis_update_metadata(s, idx);
>>> + }
>>> +
>>> + if (flags & VORBIS_FLAG_HEADER) {
>>> + ret = vorbis_parse_header(s, s->streams[idx], os->buf +
>>> os->pstart, os->psize);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + if (!os->new_extradata) {
>>> + os->new_extradata =
>>> av_mallocz(sizeof(vorbis_new_extradata));
>>> + if (!os->new_extradata)
>>> + return AVERROR(ENOMEM);
>>> + }
>>> +
>>> + os->new_extradata_size = sizeof(vorbis_new_extradata);
>>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
>>> +
>>> + ret = av_reallocp(&new_extradata->header, os->psize);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + memcpy(new_extradata->header, os->buf + os->pstart,
>>> os->psize);
>>> + new_extradata->header_size = os->psize;
>>> +
>>> + skip_packet = 1;
>>> + }
>>> +
>>> + if (flags & VORBIS_FLAG_COMMENT) {
>>> + ret = vorbis_update_metadata(s, idx);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> flags = 0;
>>> + skip_packet = 1;
>>> + }
>>> +
>>> + if (flags & VORBIS_FLAG_SETUP) {
>>> + if (!os->new_extradata) {
>>> + os->new_extradata =
>>> av_mallocz(sizeof(vorbis_new_extradata));
>>> + if (!os->new_extradata)
>>> + return AVERROR(ENOMEM);
>>> + }
>>> +
>>> + os->new_extradata_size = sizeof(vorbis_new_extradata);
>>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
>>> +
>>> + ret = av_reallocp(&new_extradata->setup, os->psize);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + memcpy(new_extradata->setup, os->buf + os->pstart, os->psize);
>>> + new_extradata->setup_size = os->psize;
>>> +
>>> + skip_packet = 1;
>>> }
>>> +
>>> os->pduration = duration;
>>> }
>>>
>>> @@ -521,7 +582,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>>> priv->final_duration += os->pduration;
>>> }
>>>
>>> - return 0;
>>> + return skip_packet;
>>> }
>>>
>>> const struct ogg_codec ff_vorbis_codec = {
>>
>> There are multiple issues with this patch:
>
> Thank you for your feedback.
>
>> 1. The side data structures are not padded, leading to
>> heap-buffer-overflows in the fate-ogg-vorbis-chained-meta test.
>
> Do you have a pointer to this issue? Is there a failing test here:
> https://fate.ffmpeg.org/ ?
>
I noted it when I ran FATE with (Clang-)ASAN locally. Seems like none of
the ASAN/valgrind fate boxes tested your commit.
>> 2. The side data structures are not flat and therefore not suitable for
>> use as AVPacketSideData. (The setup and header arrays are currently
>> owned by the demuxer, yet an AVPacket is supposed to be valid on its
>> own. But this side data becomes invalid when the demuxer encounters a
>> new side data (and reallocates its internal buffers) or when the demuxer
>> is closed.)
>
> I can work on that.
Actually, thinking about this a bit more: New extradata via side data
should use the same format as ordinary extradata, so there is no need to
add a new struct and APIchanges for that.
- Andreas
_______________________________________________
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".
