[FFmpeg-devel] [PATCH]lavf/dashdec: Do not use memcpy() to copy a struct

[wm4]

On Wed, 18 Apr 2018 16:10:26 -0300
James Almer <jamrial at gmail.com> wrote:

> On 4/18/2018 2:45 PM, Carl Eugen Hoyos wrote:
> > Hi!
> > 
> > Attached patch is supposed to fix a warning (and a bug), is this the
> > right and preferred fix?
> > 
> > Please comment, Carl Eugen
> > 
> > 
> > 0001-lavf-dashdec-Do-not-use-memcpy-to-copy-a-struct.patch
> > 
> > 
> > From cf7d2aefc1a3b3a2e9f578ede43906ed6ee96bfd Mon Sep 17 00:00:00 2001
> > From: Carl Eugen Hoyos <ceffmpeg at gmail.com>
> > Date: Wed, 18 Apr 2018 19:42:57 +0200
> > Subject: [PATCH] lavf/dashdec: Do not use memcpy() to copy a struct.
> > 
> > Fixes a warning:
> > libavformat/dashdec.c:1900:65: warning: argument to 'sizeof' in 'memcpy' call is the same pointer type 'struct fragment *' as the destination; expected 'struct fragment' or an explicit length
> > ---
> >  libavformat/dashdec.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > index 6304ad9..917fb54 100644
> > --- a/libavformat/dashdec.c
> > +++ b/libavformat/dashdec.c
> > @@ -1897,7 +1897,7 @@ static int init_section_compare_audio(DASHContext *c)
> >  
> >  static void copy_init_section(struct representation *rep_dest, struct representation *rep_src)
> >  {
> > -    memcpy(rep_dest->init_section, rep_src->init_section, sizeof(rep_src->init_section));
> > +    rep_dest->init_section = rep_src->init_section;  
> 
> This would only copy the pointer. The fact memcpy was used here makes me
> think the intention was to copy the contents of the struct, so something
> like
> 
> *rep_dest->init_section = *rep_src->init_section;
> 
> or
> 
> memcpy(rep_dest->init_section, rep_src->init_section,
> sizeof(*rep_src->init_section));
> 
> Would be the correct fix.

The first version would be preferable. But I think the original code
makes no sense and was never really tested. Looking slightly closer at
the code, init_section points to a struct that contains a further
pointer, which would require allocating and dup'ing the memory.

Also the rep_dest->init_sec_buf allocation call isn't even checked. It
just memcpy's to a NULL pointer. This is some seriously shit code, and
all of dashdec.c is shit. I'd like to ask Steven Liu (who
reviewed/pushed the patch that added this copy_init_section code) to
_actually_ review the patches and to keep up the quality standards in
this project (which are slightly higher than this).