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

Carl Eugen Hoyos ceffmpeg at gmail.com
Sat Apr 21 23:55:33 EEST 2018


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

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

You have to either fix the most obvious issues or revert the patch.

Carl Eugen


More information about the ffmpeg-devel mailing list