diff mbox

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

Message ID 1494218184-17850-6-git-send-email-rodger.combs@gmail.com
State Superseded
Headers show

Commit Message

Rodger Combs May 8, 2017, 4:36 a.m. UTC
---
 libavformat/flacenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer May 9, 2017, 10:12 a.m. UTC | #1
On Sun, May 07, 2017 at 11:36:23PM -0500, 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 9bb4947..b8800cc 100644
> --- a/libavformat/flacenc.c
> +++ b/libavformat/flacenc.c
> @@ -315,7 +315,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)) {

storing extradata in one of 2 different places is bad
i know its not added by your patch but instead of adding more code
on top of that. Please store a pointer to the streaminfo/extradata in
one place first instead of duplicating the A vs B check.

[...]
James Almer May 12, 2017, 6:28 p.m. UTC | #2
On 5/8/2017 1:36 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 9bb4947..b8800cc 100644
> --- a/libavformat/flacenc.c
> +++ b/libavformat/flacenc.c
> @@ -315,7 +315,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)) {

Instead of this, ff_flac_write_header() in flacenc_header.c should make
sure that extradata_size is exactly FLAC_STREAMINFO_SIZE and not equal
or greater.

And for that matter, this function shouldn't bother trying to rewrite
the codecpar extradata. The point of this code here is to write the new
extradata sent as part of the last packet by the flac encoder and stored
in c->streaminfo if available, not pointlessly write the original one a
second time.

>          /* rewrite the STREAMINFO header block data */
>          file_size = avio_tell(pb);
>          avio_seek(pb, 8, SEEK_SET);
>
diff mbox

Patch

diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
index 9bb4947..b8800cc 100644
--- a/libavformat/flacenc.c
+++ b/libavformat/flacenc.c
@@ -315,7 +315,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)) {
         /* rewrite the STREAMINFO header block data */
         file_size = avio_tell(pb);
         avio_seek(pb, 8, SEEK_SET);