[FFmpeg-devel] Fix memset size on ctts_data in mov_read_trun()

[Michael Niedermayer]

On Fri, Feb 23, 2018 at 05:12:05PM -0800, Xiaohan Wang (王消寒) wrote:
> Michael: Dale and I dig into history a bit more and we don't understand why
> the code is changed to the current state around memset. This new patch
> reverted the change back to the previous state which we felt is much
> cleaner. Please see the CL description for details and take a look at the
> new patch. Thanks!
> 
> On Wed, Feb 21, 2018 at 1:14 PM, Xiaohan Wang (王消寒) <xhwang at chromium.org>
> wrote:
> 
> > jstebbins: kindly ping!
> >
> > On Fri, Feb 16, 2018 at 2:42 PM, Xiaohan Wang (王消寒) <xhwang at chromium.org>
> > wrote:
> >
> >> +jstebbins@ who wrote that code.
> >>
> >> On Fri, Feb 16, 2018 at 12:30 PM, Michael Niedermayer <
> >> michael at niedermayer.cc> wrote:
> >>
> >>> On Thu, Feb 15, 2018 at 12:10:33PM -0800, Xiaohan Wang (王消寒) wrote:
> >>> >
> >>>
> >>> >  mov.c |    3 ++-
> >>> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >>> > 5597d0b095f8b15eb11503010a51c2bc2c022413
> >>> 0001-ffmpeg-Fix-memset-size-on-ctts_data-in-mov_read_trun.patch
> >>> > From 7c1e6b50ebe35b2a38c4f1d0a988e31eccbd0ead Mon Sep 17 00:00:00 2001
> >>> > From: Xiaohan Wang <xhwang at chromium.org>
> >>> > Date: Thu, 15 Feb 2018 12:05:53 -0800
> >>> > Subject: [PATCH] ffmpeg: Fix memset size on ctts_data in
> >>> mov_read_trun()
> >>> >
> >>> > The allocated size of sc->ctts_data is
> >>> > (st->nb_index_entries + entries) * sizeof(*sc->ctts_data).
> >>> >
> >>> > The size to memset at offset sc->ctts_data + sc->ctts_count should be
> >>> > (st->nb_index_entries + entries - sc->ctts_count) *
> >>> sizeof(*sc->ctts_data))
> >>> >
> >>> > The current code missed |entries| I believe.
> >>>
> >>> shouldnt "entries" be read by this function later and so shouldnt need a
> >>> memset?
> >>> I didnt write this, but it looks a bit to me as if it was intended to
> >>> only
> >>> clear the area that would not be read later
> >>>
> >>
> >> I thought we only had sc->ctts_count entries before av_fast_realloc, so
> >> memset everything starting from sc->ctts_data + sc->ctts_count couldn't
> >> go wrong. But I am not familiar with this code and that could totally be
> >> wrong. I added jstebbins@ who wrote the code and hopefully we can get
> >> expert opinion there.
> >>
> >>
> >>> [...]
> >>> --
> >>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >>>
> >>> No great genius has ever existed without some touch of madness. --
> >>> Aristotle
> >>>
> >>> _______________________________________________
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel at ffmpeg.org
> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>
> >>>
> >>
> >

>  mov.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> e5bbe55f0b1260f787f431b5c45e6ca49a7d2d1e  0001-Fix-memset-size-on-ctts_data-in-mov_read_trun-round-.patch
> From f34b35ec5749c17b21f80665a0b8859bce3e84ab Mon Sep 17 00:00:00 2001
> From: Xiaohan Wang <xhwang at chromium.org>
> Date: Fri, 23 Feb 2018 10:51:30 -0800
> Subject: [PATCH] Fix memset size on ctts_data in mov_read_trun() (round 2)
> 
> The allocated size of sc->ctts_data is
> (st->nb_index_entries + entries) * sizeof(*sc->ctts_data).
> 
> The size to memset at offset sc->ctts_data + sc->ctts_count should be
> (st->nb_index_entries + entries - sc->ctts_count) *
> sizeof(*sc->ctts_data))
> 
> The current code missed |entries| I believe, which was introduced in
> https://patchwork.ffmpeg.org/patch/5541/.
> 
> However, after offline discussion, it seems the original code is much
> more clear to read (before https://patchwork.ffmpeg.org/patch/5541/).
> 
> Hence this CL revert the memset logic to it's previous state by
> remembering the |old_ctts_allocated_size|, and only memset the newly
> allocated entries.
> ---
>  libavformat/mov.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index a3725692a7..f8d79c7b02 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4713,6 +4713,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      st->index_entries= new_entries;
>  
>      requested_size = (st->nb_index_entries + entries) * sizeof(*sc->ctts_data);
> +    unsigned int old_ctts_allocated_size = sc->ctts_allocated_size;

this should possibly be size_t 

and declaration and statements should not be mixed

libavformat/mov.c: In function ‘mov_read_trun’:
libavformat/mov.c:4691:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180225/77d406d3/attachment.sig>