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

[Liu Steven]

> 在 2018年4月22日，上午5:23，wm4 <nfxjfg at googlemail.com> 写道：
> 
> 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.
The problem is how do you check i have not check the patch is ok or not?
Only myself review the patch, where are the other guys when i response LGTM?
you guys can objections the patch, but no, isn’t it?
> 
>> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel