Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-18 Thread Michael Niedermayer 
On Wed, Mar 18, 2015 at 10:01:36PM +0100, Andreas Cadhalpun wrote:
> On 18.03.2015 16:59, tomas.har...@codemill.se wrote:
> > On 2015-03-17 16:13, Andreas Cadhalpun wrote:
> >> On 17.03.2015 10:17, tomas.har...@codemill.se wrote:
> >>> On 2015-03-14 18:03, Andreas Cadhalpun wrote:
> >>> [PATCH 2/2] mxfenc: don't try to write footer without header:
> >>>
>  +if (!mxf->header_written ||
>  +(s->oformat == &ff_mxf_opatom_muxer && 
>  !mxf->body_partition_offset)) {
>  +err = AVERROR_UNKNOWN;
>  +goto end;
>  +}
>  +
> >>>
> >>> AVERROR_UNKNOWN?
> >>
> >> It's unclear why the header was not written or body_partition_offset
> >> not allocated. It could e.g. be due to invalid options, not supported
> >> codecs, or just out of memory.
> >> Do you think AVERROR(EINVAL) or even just -1 would be better?
> >>
> >> Best regards,
> >> Andreas
> > 
> > No preference really. Perhaps a comment though?
> 
> OK. New patch with comment attached.
> 
> Best regards,
> Andreas
> 

>  mxfenc.c |7 +++
>  1 file changed, 7 insertions(+)
> db5cf2fdfe8a815e8785669644690907a9a5a7fe  
> 0001-mxfenc-don-t-try-to-write-footer-without-header.patch
> From 1424c87b9a786555b294fb9daa9fd2a67be9a30d Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun 
> Date: Wed, 18 Mar 2015 21:57:58 +0100
> Subject: [PATCH] mxfenc: don't try to write footer without header
> 
> This fixes a crash, when trying to mux h264 into mxf_opatom.

applied

thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle


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

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-18 Thread Andreas Cadhalpun 
On 18.03.2015 16:59, tomas.har...@codemill.se wrote:
> On 2015-03-17 16:13, Andreas Cadhalpun wrote:
>> On 17.03.2015 10:17, tomas.har...@codemill.se wrote:
>>> On 2015-03-14 18:03, Andreas Cadhalpun wrote:
>>> [PATCH 2/2] mxfenc: don't try to write footer without header:
>>>
 +if (!mxf->header_written ||
 +(s->oformat == &ff_mxf_opatom_muxer && 
 !mxf->body_partition_offset)) {
 +err = AVERROR_UNKNOWN;
 +goto end;
 +}
 +
>>>
>>> AVERROR_UNKNOWN?
>>
>> It's unclear why the header was not written or body_partition_offset
>> not allocated. It could e.g. be due to invalid options, not supported
>> codecs, or just out of memory.
>> Do you think AVERROR(EINVAL) or even just -1 would be better?
>>
>> Best regards,
>> Andreas
> 
> No preference really. Perhaps a comment though?

OK. New patch with comment attached.

Best regards,
Andreas

>From 1424c87b9a786555b294fb9daa9fd2a67be9a30d Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Wed, 18 Mar 2015 21:57:58 +0100
Subject: [PATCH] mxfenc: don't try to write footer without header

This fixes a crash, when trying to mux h264 into mxf_opatom.

Signed-off-by: Andreas Cadhalpun 
---
 libavformat/mxfenc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index e9c4a9d..0349e5d 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2402,6 +2402,13 @@ static int mxf_write_footer(AVFormatContext *s)
 AVIOContext *pb = s->pb;
 int err = 0;
 
+if (!mxf->header_written ||
+(s->oformat == &ff_mxf_opatom_muxer && !mxf->body_partition_offset)) {
+/* reason could be invalid options/not supported codec/out of memory */
+err = AVERROR_UNKNOWN;
+goto end;
+}
+
 mxf->duration = mxf->last_indexed_edit_unit + mxf->edit_units_count;
 
 mxf_write_klv_fill(s);
-- 
2.1.4

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

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-18 Thread tomas . hardin 

On 2015-03-17 16:13, Andreas Cadhalpun wrote:

On 17.03.2015 10:17, tomas.har...@codemill.se wrote:

On 2015-03-14 18:03, Andreas Cadhalpun wrote:
[PATCH 2/2] mxfenc: don't try to write footer without header:


+if (!mxf->header_written ||
+(s->oformat == &ff_mxf_opatom_muxer && 
!mxf->body_partition_offset)) {

+err = AVERROR_UNKNOWN;
+goto end;
+}
+


AVERROR_UNKNOWN?


It's unclear why the header was not written or body_partition_offset
not allocated. It could e.g. be due to invalid options, not supported
codecs, or just out of memory.
Do you think AVERROR(EINVAL) or even just -1 would be better?

Best regards,
Andreas


No preference really. Perhaps a comment though?

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

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-17 Thread Andreas Cadhalpun 
On 17.03.2015 10:17, tomas.har...@codemill.se wrote:
> On 2015-03-14 18:03, Andreas Cadhalpun wrote:
> [PATCH 2/2] mxfenc: don't try to write footer without header:
> 
>> +if (!mxf->header_written ||
>> +(s->oformat == &ff_mxf_opatom_muxer && 
>> !mxf->body_partition_offset)) {
>> +err = AVERROR_UNKNOWN;
>> +goto end;
>> +}
>> +
> 
> AVERROR_UNKNOWN?

It's unclear why the header was not written or body_partition_offset
not allocated. It could e.g. be due to invalid options, not supported
codecs, or just out of memory.
Do you think AVERROR(EINVAL) or even just -1 would be better?

Best regards,
Andreas

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

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-17 Thread Michael Niedermayer 
On Tue, Mar 17, 2015 at 10:17:06AM +0100, tomas.har...@codemill.se wrote:
> On 2015-03-14 18:03, Andreas Cadhalpun wrote:
> >On 14.03.2015 02:17, Mark Reid wrote:
> >>On Fri, Mar 13, 2015 at 6:02 AM, Andreas Cadhalpun <
> >>andreas.cadhal...@googlemail.com> wrote:
[...]

> Memleak patch is obviously OK.

applied

thanks

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

DNS cache poisoning attacks, popular search engine, Google internet authority
dont be evil, please


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

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-17 Thread tomas . hardin 

On 2015-03-14 18:03, Andreas Cadhalpun wrote:

On 14.03.2015 02:17, Mark Reid wrote:

On Fri, Mar 13, 2015 at 6:02 AM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:


On 13.03.2015 11:59, Tomas Härdin wrote:

A better solution would
be to figure out why mxf->body_partition_offset becomes NULL so that
index tables and such can be rewritten properly.


It can always happen that mxf->body_partition_offset is NULL, e.g. if
no memory is left, or if something else fails. Try e.g.:
ffmpeg -f lavfi -i testsrc -c:v libx264 -f mxf_opatom


mxf->body_partition_offset is NULL because currently only AVC Intra 
50/100

h264 is supported.


Yes.


The encoder figures out the h264 format by parsing the
h264 packet and doesn't write the body partiton (or even the header
partition) untill after it parses the first packet. If the packet is
invalid, nothing get written and mxf->body_partition_offset doesn't 
get

allocated.


That's correct.


 perhaps mxf_write_footer should return a error if
mxf->body_partition_offset is NULL or just if mxf->header_written == 0
 before doing trying to write anything.


Well, mxf_write_footer also has to free some allocated memory, which 
would

get leaked if one just returns...
...like it does in any of the currently present 'return err' cases.

Attached is a patch fixing the memleaks and another returning an error,
if no header was written before the footer.

Best regards,
Andreas


[PATCH 2/2] mxfenc: don't try to write footer without header:


+if (!mxf->header_written ||
+(s->oformat == &ff_mxf_opatom_muxer && 
!mxf->body_partition_offset)) {

+err = AVERROR_UNKNOWN;
+goto end;
+}
+


AVERROR_UNKNOWN?

Memleak patch is obviously OK.

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

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-14 Thread Andreas Cadhalpun 
On 14.03.2015 02:17, Mark Reid wrote:
> On Fri, Mar 13, 2015 at 6:02 AM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> On 13.03.2015 11:59, Tomas Härdin wrote:
>>> A better solution would
>>> be to figure out why mxf->body_partition_offset becomes NULL so that
>>> index tables and such can be rewritten properly.
>>
>> It can always happen that mxf->body_partition_offset is NULL, e.g. if
>> no memory is left, or if something else fails. Try e.g.:
>> ffmpeg -f lavfi -i testsrc -c:v libx264 -f mxf_opatom
>>
>>
> mxf->body_partition_offset is NULL because currently only AVC Intra 50/100
> h264 is supported.

Yes.

> The encoder figures out the h264 format by parsing the
> h264 packet and doesn't write the body partiton (or even the header
> partition) untill after it parses the first packet. If the packet is
> invalid, nothing get written and mxf->body_partition_offset doesn't get
> allocated.

That's correct.

>  perhaps mxf_write_footer should return a error if
> mxf->body_partition_offset is NULL or just if mxf->header_written == 0
>  before doing trying to write anything.

Well, mxf_write_footer also has to free some allocated memory, which would
get leaked if one just returns...
...like it does in any of the currently present 'return err' cases.

Attached is a patch fixing the memleaks and another returning an error,
if no header was written before the footer.

Best regards,
Andreas
>From ac6970e62ddd65fe18be10f1c79f10c6d8ce3a75 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Sat, 14 Mar 2015 17:48:56 +0100
Subject: [PATCH 2/2] mxfenc: don't try to write footer without header

This fixes a crash, when trying to mux h264 into mxf_opatom.

Signed-off-by: Andreas Cadhalpun 
---
 libavformat/mxfenc.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index bf680f8..2485741 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2340,6 +2340,12 @@ static int mxf_write_footer(AVFormatContext *s)
 AVIOContext *pb = s->pb;
 int err = 0;
 
+if (!mxf->header_written ||
+(s->oformat == &ff_mxf_opatom_muxer && !mxf->body_partition_offset)) {
+err = AVERROR_UNKNOWN;
+goto end;
+}
+
 mxf->duration = mxf->last_indexed_edit_unit + mxf->edit_units_count;
 
 mxf_write_klv_fill(s);
-- 
2.1.4

>From 5d9c6182f3f203fcbb286012d13cff76f9083b57 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Sat, 14 Mar 2015 17:47:53 +0100
Subject: [PATCH 1/2] mxfenc: fix memleaks in mxf_write_footer

Signed-off-by: Andreas Cadhalpun 
---
 libavformat/mxfenc.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 898951c..bf680f8 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2338,7 +2338,7 @@ static int mxf_write_footer(AVFormatContext *s)
 {
 MXFContext *mxf = s->priv_data;
 AVIOContext *pb = s->pb;
-int err;
+int err = 0;
 
 mxf->duration = mxf->last_indexed_edit_unit + mxf->edit_units_count;
 
@@ -2346,10 +2346,10 @@ static int mxf_write_footer(AVFormatContext *s)
 mxf->footer_partition_offset = avio_tell(pb);
 if (mxf->edit_unit_byte_count && s->oformat != &ff_mxf_opatom_muxer) { // no need to repeat index
 if ((err = mxf_write_partition(s, 0, 0, footer_partition_key, 0)) < 0)
-return err;
+goto end;
 } else {
 if ((err = mxf_write_partition(s, 0, 2, footer_partition_key, 0)) < 0)
-return err;
+goto end;
 mxf_write_klv_fill(s);
 mxf_write_index_table_segment(s);
 }
@@ -2362,21 +2362,22 @@ static int mxf_write_footer(AVFormatContext *s)
 /* rewrite body partition to update lengths */
 avio_seek(pb, mxf->body_partition_offset[0], SEEK_SET);
 if ((err = mxf_write_opatom_body_partition(s)) < 0)
-return err;
+goto end;
 }
 
 avio_seek(pb, 0, SEEK_SET);
 if (mxf->edit_unit_byte_count && s->oformat != &ff_mxf_opatom_muxer) {
 if ((err = mxf_write_partition(s, 1, 2, header_closed_partition_key, 1)) < 0)
-return err;
+goto end;
 mxf_write_klv_fill(s);
 mxf_write_index_table_segment(s);
 } else {
 if ((err = mxf_write_partition(s, 0, 0, header_closed_partition_key, 1)) < 0)
-return err;
+goto end;
 }
 }
 
+end:
 ff_audio_interleave_close(s);
 
 av_freep(&mxf->index_entries);
@@ -2386,7 +2387,7 @@ static int mxf_write_footer(AVFormatContext *s)
 
 mxf_free(s);
 
-return 0;
+return err < 0 ? err : 0;
 }
 
 static int mxf_interleave_get_packet(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush)
-- 
2.1.4

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

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-13 Thread Mark Reid 
On Fri, Mar 13, 2015 at 6:02 AM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 13.03.2015 11:59, Tomas Härdin wrote:
> > On Thu, 2015-03-12 at 17:48 +0100, Andreas Cadhalpun wrote:
> >> This fixes a crash, when trying to mux h264 into mxf_opatom.
> >>
> >> Signed-off-by: Andreas Cadhalpun 
> >> ---
> >>  libavformat/mxfenc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> >> index 898951c..2891f5d 100644
> >> --- a/libavformat/mxfenc.c
> >> +++ b/libavformat/mxfenc.c
> >> @@ -2358,7 +2358,7 @@ static int mxf_write_footer(AVFormatContext *s)
> >>  mxf_write_random_index_pack(s);
> >>
> >>  if (s->pb->seekable) {
> >> -if (s->oformat == &ff_mxf_opatom_muxer){
> >> +if (s->oformat == &ff_mxf_opatom_muxer &&
> mxf->body_partition_offset){
> >>  /* rewrite body partition to update lengths */
> >>  avio_seek(pb, mxf->body_partition_offset[0], SEEK_SET);
> >>  if ((err = mxf_write_opatom_body_partition(s)) < 0)
> >
> > Doesn't this need to happen for H.264 as well?
>
> Maybe, but the seek can't work if mxf->body_partition_offset is NULL.
> Would it be better to add the check only around the seek?
>
> > A better solution would
> > be to figure out why mxf->body_partition_offset becomes NULL so that
> > index tables and such can be rewritten properly.
>
> It can always happen that mxf->body_partition_offset is NULL, e.g. if
> no memory is left, or if something else fails. Try e.g.:
> ffmpeg -f lavfi -i testsrc -c:v libx264 -f mxf_opatom
>
>
mxf->body_partition_offset is NULL because currently only AVC Intra 50/100
h264 is supported. The encoder figures out the h264 format by parsing the
h264 packet and doesn't write the body partiton (or even the header
partition) untill after it parses the first packet. If the packet is
invalid, nothing get written and mxf->body_partition_offset doesn't get
allocated.  perhaps mxf_write_footer should return a error if
mxf->body_partition_offset is NULL or just if mxf->header_written == 0
 before doing trying to write anything.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-13 Thread Andreas Cadhalpun 
On 13.03.2015 11:59, Tomas Härdin wrote:
> On Thu, 2015-03-12 at 17:48 +0100, Andreas Cadhalpun wrote:
>> This fixes a crash, when trying to mux h264 into mxf_opatom.
>>
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavformat/mxfenc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> index 898951c..2891f5d 100644
>> --- a/libavformat/mxfenc.c
>> +++ b/libavformat/mxfenc.c
>> @@ -2358,7 +2358,7 @@ static int mxf_write_footer(AVFormatContext *s)
>>  mxf_write_random_index_pack(s);
>>
>>  if (s->pb->seekable) {
>> -if (s->oformat == &ff_mxf_opatom_muxer){
>> +if (s->oformat == &ff_mxf_opatom_muxer && 
>> mxf->body_partition_offset){
>>  /* rewrite body partition to update lengths */
>>  avio_seek(pb, mxf->body_partition_offset[0], SEEK_SET);
>>  if ((err = mxf_write_opatom_body_partition(s)) < 0)
> 
> Doesn't this need to happen for H.264 as well?

Maybe, but the seek can't work if mxf->body_partition_offset is NULL.
Would it be better to add the check only around the seek?

> A better solution would
> be to figure out why mxf->body_partition_offset becomes NULL so that
> index tables and such can be rewritten properly.

It can always happen that mxf->body_partition_offset is NULL, e.g. if
no memory is left, or if something else fails. Try e.g.:
ffmpeg -f lavfi -i testsrc -c:v libx264 -f mxf_opatom

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mxfenc: ensure mxf->body_partition_offset is not NULL before using it

2015-03-13 Thread Tomas Härdin 
On Thu, 2015-03-12 at 17:48 +0100, Andreas Cadhalpun wrote:
> This fixes a crash, when trying to mux h264 into mxf_opatom.
> 
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavformat/mxfenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 898951c..2891f5d 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2358,7 +2358,7 @@ static int mxf_write_footer(AVFormatContext *s)
>  mxf_write_random_index_pack(s);
> 
>  if (s->pb->seekable) {
> -if (s->oformat == &ff_mxf_opatom_muxer){
> +if (s->oformat == &ff_mxf_opatom_muxer && 
> mxf->body_partition_offset){
>  /* rewrite body partition to update lengths */
>  avio_seek(pb, mxf->body_partition_offset[0], SEEK_SET);
>  if ((err = mxf_write_opatom_body_partition(s)) < 0)

Doesn't this need to happen for H.264 as well? A better solution would
be to figure out why mxf->body_partition_offset becomes NULL so that
index tables and such can be rewritten properly.

/Tomas



signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel