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

wm4 nfxjfg at googlemail.com
Sun Apr 22 00:23:42 EEST 2018


On Sat, 21 Apr 2018 22:55:33 +0200
Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:

> 2018-04-19 4:45 GMT+02:00, Steven Liu <lq at chinaffmpeg.org>:
> >
> >  
> >> On 19 Apr 2018, at 03:20, wm4 <nfxjfg at googlemail.com> wrote:
> >>
> >> 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).  
> > Yes, that is my mistake, patch welcome and welcome you to contribute code
> > for refine the dashdec  

The problem was that you didn't actually review the patch. There's
really no excuse for the code that has been added. It's not even valid
C. It's sort of your responsibility to make sure this doesn't happen.
Sorry if my words were a bit too direct, but for very new code dashdec
has a bit too many issues than it should have. Reviewing means more
than just replying "LGTM" and pushing a patch.

> No (independently of what I think of Vincent's character and tone).

Agreed, independently of what I think of you.

Just by the way, some have lamented that they think of it as "doxing"
when you post my real name on this list. Do you think this is acceptable
behavior? Don't worry, my real name has been on my github profile for
years (and before that on the mplayer2 website), in both cases visible
to anyone, so you don't have to fear that your childish attempts to
achieve whatever have any effect.


More information about the ffmpeg-devel mailing list