diff mbox series

[FFmpeg-devel,11/12] avformat/nutenc: Don't segfault when chapters are added during muxing

Message ID 20200505141657.10787-1-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,01/10] libavformat/nutenc: Remove redundant function parameter
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt May 5, 2020, 2:16 p.m. UTC
When writing the header, the NUT muxer allocates an array with as many
entries as there are chapters containing information about the used
timebase. This information is used when writing the headers and also
when resending the headers (as the NUT muxer does from time to time).

When the NUT muxer writes or resends the headers, it simply presumes
that there are enough entries in its array for each chapter in the
AVFormatContext. Yet users are allowed to add chapters during the muxing
process, so this presumption is wrong and may lead to segfaults.

So explicitly store the number of entries of the chapter array and refer
to this number whenever headers are written.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
This patch presumes that the user may not change or remove the chapters
available during writing the header (if there were chapters available
when writing the header at all). I hope this is ok.

 libavformat/nut.h    | 1 +
 libavformat/nutenc.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer May 7, 2020, 3:15 p.m. UTC | #1
On Tue, May 05, 2020 at 04:16:56PM +0200, Andreas Rheinhardt wrote:
> When writing the header, the NUT muxer allocates an array with as many
> entries as there are chapters containing information about the used
> timebase. This information is used when writing the headers and also
> when resending the headers (as the NUT muxer does from time to time).
> 
> When the NUT muxer writes or resends the headers, it simply presumes
> that there are enough entries in its array for each chapter in the
> AVFormatContext. Yet users are allowed to add chapters during the muxing
> process, so this presumption is wrong and may lead to segfaults.
> 
> So explicitly store the number of entries of the chapter array and refer
> to this number whenever headers are written.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> This patch presumes that the user may not change or remove the chapters
> available during writing the header (if there were chapters available
> when writing the header at all). I hope this is ok.
> 
>  libavformat/nut.h    | 1 +
>  libavformat/nutenc.c | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)

how do i apply this (for testing) ?
on its own it fails and it seems the previous patchset doesnt like applying
anymore either 


> 
> diff --git a/libavformat/nut.h b/libavformat/nut.h
> index a4409ee23d..52225fed93 100644
> --- a/libavformat/nut.h
> +++ b/libavformat/nut.h
> @@ -115,6 +115,7 @@ typedef struct NUTContext {
>      int flags;
>      int version; // version currently in use
>      int minor_version;
> +    unsigned nb_chapters;
>  } NUTContext;
>  
>  extern const AVCodecTag ff_nut_subtitle_tags[];
> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
> index 5071278835..2d35c44b79 100644
> --- a/libavformat/nutenc.c
> +++ b/libavformat/nutenc.c
> @@ -675,7 +675,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc)
>              goto fail;
>      }
>  
> -    for (i = 0; i < nut->avf->nb_chapters; i++) {
> +    for (i = 0; i < nut->nb_chapters; i++) {
>          write_chapter(nut, dyn_bc, i, prelude, &prelude_size);

also if i read this correctly, this would not write all chapters.
That seems not ideal

Thanks

[...]
Andreas Rheinhardt May 7, 2020, 3:49 p.m. UTC | #2
Michael Niedermayer:
> On Tue, May 05, 2020 at 04:16:56PM +0200, Andreas Rheinhardt wrote:
>> When writing the header, the NUT muxer allocates an array with as many
>> entries as there are chapters containing information about the used
>> timebase. This information is used when writing the headers and also
>> when resending the headers (as the NUT muxer does from time to time).
>>
>> When the NUT muxer writes or resends the headers, it simply presumes
>> that there are enough entries in its array for each chapter in the
>> AVFormatContext. Yet users are allowed to add chapters during the muxing
>> process, so this presumption is wrong and may lead to segfaults.
>>
>> So explicitly store the number of entries of the chapter array and refer
>> to this number whenever headers are written.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> This patch presumes that the user may not change or remove the chapters
>> available during writing the header (if there were chapters available
>> when writing the header at all). I hope this is ok.
>>
>>  libavformat/nut.h    | 1 +
>>  libavformat/nutenc.c | 3 ++-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> how do i apply this (for testing) ?

When you test this, you should be aware that the first time the header
is repeated is after 16 TiB of output (see patch #12 for the reason), so
you should better change the following line in nut_write_packet() to
reduce said interval:
    if (1LL << (20 + 3 * nut->header_count) <= avio_tell(bc))

> on its own it fails and it seems the previous patchset doesnt like applying
> anymore either 
> 
I have already applied patches 1-4 + 9 (that you said were ok) from this
patchset. Patches 5-8 and 10-11 apply cleanly on top of master. Did you
try to also apply the patches that have already been merged?

(Patch #12 does not apply cleanly any more, because of the changes to
the ref files in b4967fc71c63eae8cd96f9c46cd3e1fbd705bbf9. Furthermore
my patch as sent only updated a subset of fate-tests, because my build
was not configured with enable-gpl. I already fixed this. Normally I'd
resend the mail, but given its huge size I'd like to avoid that this
time. Should I send it?)

> 
>>
>> diff --git a/libavformat/nut.h b/libavformat/nut.h
>> index a4409ee23d..52225fed93 100644
>> --- a/libavformat/nut.h
>> +++ b/libavformat/nut.h
>> @@ -115,6 +115,7 @@ typedef struct NUTContext {
>>      int flags;
>>      int version; // version currently in use
>>      int minor_version;
>> +    unsigned nb_chapters;
>>  } NUTContext;
>>  
>>  extern const AVCodecTag ff_nut_subtitle_tags[];
>> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
>> index 5071278835..2d35c44b79 100644
>> --- a/libavformat/nutenc.c
>> +++ b/libavformat/nutenc.c
>> @@ -675,7 +675,7 @@ static int write_headers(AVFormatContext *avctx, AVIOContext *bc)
>>              goto fail;
>>      }
>>  
>> -    for (i = 0; i < nut->avf->nb_chapters; i++) {
>> +    for (i = 0; i < nut->nb_chapters; i++) {
>>          write_chapter(nut, dyn_bc, i, prelude, &prelude_size);
> 
> also if i read this correctly, this would not write all chapters.
> That seems not ideal

It's indeed not ideal; it is designed to fix a potential segfault, not
to add functionality. (Without this patch patch #12 would run into this
bug more often.)

I don't know NUT very well, but I see two ways how this could be
implemented:
1. One adds the timebases of these chapters to the arrays of timebases
that already exist and writes the chapters with their timebases. This
would involve moving code from nut_write_header() to write_headers(). I
don't know whether adding a new timebase mid-stream is even allowed by
the spec.
2. One writes the chapters with the best timebase available for them,
even when it is not an exact match.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/nut.h b/libavformat/nut.h
index a4409ee23d..52225fed93 100644
--- a/libavformat/nut.h
+++ b/libavformat/nut.h
@@ -115,6 +115,7 @@  typedef struct NUTContext {
     int flags;
     int version; // version currently in use
     int minor_version;
+    unsigned nb_chapters;
 } NUTContext;
 
 extern const AVCodecTag ff_nut_subtitle_tags[];
diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c
index 5071278835..2d35c44b79 100644
--- a/libavformat/nutenc.c
+++ b/libavformat/nutenc.c
@@ -675,7 +675,7 @@  static int write_headers(AVFormatContext *avctx, AVIOContext *bc)
             goto fail;
     }
 
-    for (i = 0; i < nut->avf->nb_chapters; i++) {
+    for (i = 0; i < nut->nb_chapters; i++) {
         write_chapter(nut, dyn_bc, i, prelude, &prelude_size);
         ret = put_packet(nut, bc, dyn_bc, prelude, prelude_size, INFO_STARTCODE);
         if (ret < 0)
@@ -719,6 +719,7 @@  static int nut_write_header(AVFormatContext *s)
         nut->chapter = av_calloc(s->nb_chapters, sizeof(*nut->chapter));
         if (!nut->chapter)
             return AVERROR(ENOMEM);
+        nut->nb_chapters = s->nb_chapters;
     }
 
     for (i = 0; i < s->nb_streams; i++) {