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

[Michael Niedermayer]

On Thu, May 07, 2020 at 05:49:06PM +0200, Andreas Rheinhardt wrote:
> 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 at 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.)

how hard is it to fix this correctly ?
if its really alot of code sure its ok to do a simple fix for something
like backporting ...


> 
> 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.

no i dont think you can add timebases mid stream currently.Iam not even sure how
much sense that makes when you cannot add streams mid stream


> 2. One writes the chapters with the best timebase available for them,
> even when it is not an exact match.

i would be interrested in cases where an exact match is not available
that said i dont care much about what happens when the user provided
chapters are not representable in the format. Error out, drop, approximate.

thx

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

Never trust a computer, one day, it may think you are the virus. -- Compn
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20201109/611747a5/attachment.sig>