[FFmpeg-devel] [PATCH 6/7] lavf/flacenc: avoid buffer overread with unexpected extradata sizes

[Rodger Combs]

> On Aug 1, 2017, at 18:25, James Almer <jamrial at gmail.com> wrote:
> 
> On 8/1/2017 3:33 AM, Rodger Combs wrote:
>> ---
>> libavformat/flacenc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
>> index 9768b6a..1906aee 100644
>> --- a/libavformat/flacenc.c
>> +++ b/libavformat/flacenc.c
>> @@ -322,7 +322,7 @@ static int flac_write_trailer(struct AVFormatContext *s)
>>     if (!c->write_header || !streaminfo)
>>         return 0;
>> 
>> -    if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
>> +    if (pb->seekable & AVIO_SEEKABLE_NORMAL && (c->streaminfo || s->streams[0]->codecpar->extradata_size == FLAC_STREAMINFO_SIZE)) {
> 
> In what situation would stream extradata or c->streaminfo be smaller
> than FLAC_STREAMINFO_SIZE? Nothing changes par->extradata anywhere, and
> c->streaminfo is always allocated with FLAC_STREAMINFO_SIZE bytes of memory.
> I have the feeling i already asked this before, but not sure if you
> answered it. Apologies if you did.

It shouldn't happen in normal cases, but you could imagine a case with remuxing a corrupted input file.

> 
> You can also simplify this by just rewriting the STREAMINFO header only
> if c->streaminfo is allocated, meaning updated extradata showed up in a
> packet. Otherwise, you'd be rewriting the same STREAMINFO header you
> already wrote at the beginning.

Ah, good idea! Done.

> You could also even use ff_flac_write_header(), which already does a
> buffer size check.

I don't think this is necessary when coupled with your previous suggestion?

> 
> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> index b894f9ef61..7392cf13c1 100644
> --- a/libavformat/flacenc.c
> +++ b/libavformat/flacenc.c
> @@ -141,17 +141,15 @@ static int flac_write_trailer(struct
> AVFormatContext *s)
>     AVIOContext *pb = s->pb;
>     int64_t file_size;
>     FlacMuxerContext *c = s->priv_data;
> -    uint8_t *streaminfo = c->streaminfo ? c->streaminfo :
> -
> s->streams[0]->codecpar->extradata;
> 
> -    if (!c->write_header || !streaminfo)
> +    if (!c->write_header || !c->streaminfo)
>         return 0;
> 
>     if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
>         /* rewrite the STREAMINFO header block data */
>         file_size = avio_tell(pb);
> -        avio_seek(pb, 8, SEEK_SET);
> -        avio_write(pb, streaminfo, FLAC_STREAMINFO_SIZE);
> +        avio_seek(pb, 0, SEEK_SET);
> +        ff_flac_write_header(pb, c->streaminfo, FLAC_STREAMINFO_SIZE, 0);
>         avio_seek(pb, file_size, SEEK_SET);
>         avio_flush(pb);
>     } else {
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel