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

2017-11-21 Thread Rodger Combs 


> On Nov 21, 2017, at 19:03, Rostislav Pehlivanov  wrote:
> 
> On 2 August 2017 at 00:35, Rodger Combs  > wrote:
> 
>> 
>>> On Aug 1, 2017, at 18:25, James Almer  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.
>> 
> 
> Shouldn't the demuxer handle this?

Even in e.g. MKV?

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org 
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 
> 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-11-21 Thread Rostislav Pehlivanov 
On 2 August 2017 at 00:35, Rodger Combs  wrote:

>
> > On Aug 1, 2017, at 18:25, James Almer  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.
>

Shouldn't the demuxer handle this?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-08-02 Thread Michael Niedermayer 
On Wed, Aug 02, 2017 at 01:30:44AM -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 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)) {


this looks a bit odd

uint8_t *streaminfo = c->streaminfo ? c->streaminfo :
  s->streams[0]->codecpar->extradata;
...
> +if (pb->seekable & AVIO_SEEKABLE_NORMAL && (c->streaminfo || 
> s->streams[0]->codecpar->extradata_size == FLAC_STREAMINFO_SIZE)) {

isnt this just "&& c->streaminfo" ?

also is s->streams[0] correct ?
shouldnt this use c->audio_stream_idx ?

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-08-02 Thread Rodger Combs 
---
 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)) {
 /* rewrite the STREAMINFO header block data */
 file_size = avio_tell(pb);
 avio_seek(pb, 8, SEEK_SET);
-- 
2.6.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-08-01 Thread Rodger Combs 

> On Aug 1, 2017, at 18:25, James Almer  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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-08-01 Thread James Almer 
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.

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.
You could also even use ff_flac_write_header(), which already does a
buffer size check.

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@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-08-01 Thread Rodger Combs 
---
 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)) {
 /* rewrite the STREAMINFO header block data */
 file_size = avio_tell(pb);
 avio_seek(pb, 8, SEEK_SET);
-- 
2.6.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-05-12 Thread James Almer 
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);
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-05-09 Thread Michael Niedermayer 
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.

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-05-07 Thread Rodger Combs 
---
 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)) {
 /* rewrite the STREAMINFO header block data */
 file_size = avio_tell(pb);
 avio_seek(pb, 8, SEEK_SET);
-- 
2.6.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel