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

Hi!

Attached patch is supposed to fix a warning (and a bug), is this the
right and preferred fix?

Please comment, Carl Eugen

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@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.

>      rep_dest->init_sec_buf = av_mallocz(rep_src->init_sec_buf_size);
>      memcpy(rep_dest->init_sec_buf, rep_src->init_sec_buf, rep_src->init_sec_data_len);
>      rep_dest->init_sec_buf_size = rep_src->init_sec_buf_size;
> -- 1.7.10.4
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On Wed, 18 Apr 2018 16:10:26 -0300
James Almer <jamrial@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@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).

> On 19 Apr 2018, at 03:20, wm4 <nfxjfg@googlemail.com> wrote:
> 
> On Wed, 18 Apr 2018 16:10:26 -0300
> James Almer <jamrial@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@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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven

2018-04-19 4:45 GMT+02:00, Steven Liu <lq@chinaffmpeg.org>:
>
>
>> On 19 Apr 2018, at 03:20, wm4 <nfxjfg@googlemail.com> wrote:
>>
>> On Wed, 18 Apr 2018 16:10:26 -0300
>> James Almer <jamrial@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@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

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

> 2018-04-19 4:45 GMT+02:00, Steven Liu <lq@chinaffmpeg.org>:
> >
> >  
> >> On 19 Apr 2018, at 03:20, wm4 <nfxjfg@googlemail.com> wrote:
> >>
> >> On Wed, 18 Apr 2018 16:10:26 -0300
> >> James Almer <jamrial@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@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.

2018-04-21 23:23 GMT+02:00, wm4 <nfxjfg@googlemail.com>:

>> 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"

What is "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.

Yes, other developers told me they found your name on github.
I believe my attempts to produce readable sentences works at least
sometimes, so I mostly achieve the effect I want.

Carl Eugen

On Sat, 21 Apr 2018 23:30:35 +0200
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2018-04-21 23:23 GMT+02:00, wm4 <nfxjfg@googlemail.com>:
> 
> >> 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"  
> 
> What is "doxing"?

Something which most codes of conduct forbid.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512


On 2018-04-21 17:23, wm4 wrote:
> Just by the way, some have lamented that they think of it as
> "doxing" when you post my real name on this list.

I don't really care, if somebody is using a pseudonym or their real name.

However, I have to say that if sombody is using not their real name,
especially when their tone is often agressive and unfiltered, it
rather begs the question why that is.

Is that person afraid that an employer might come across some of this
crap when doing research on a potential candidate?
Is that person not confident enough to stand by their opinion.

Seriously, what is it?

Btw, I myself often use blunt comments, which are mostly seen and read
as harsh, undiplomatic, and rude. I hate poltical correctness and love
sarcasm. Non of these traits are a plus in public. People love sugar
coating and talking around a problem, all in the name of diplomacy.

- -- 
regards Helmut K. C. Tessarek KeyID 0x172380A011EF4944
Key fingerprint = 8A55 70C1 BD85 D34E ADBC 386C 1723 80A0 11EF 4944

/*
Thou shalt not follow the NULL pointer for chaos and madness
await thee at its end.
*/
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEE191csiqpm8f5Ln9WvgmFNJ1E3QAFAlrbs4MACgkQvgmFNJ1E
3QDEMA//SZNScbz+ngk0UU040cWdT6Tek83Dh5RhKwfTRzY7VlvTnsQTYG/ktJn8
2yiLUvMQWh+ENr+Kxe9MosFR7kDOkD4s/P199IkmQSC0Vdh0YwLNsaVamziAFjHP
ZXbpn0v+PasU5raGbpJfQE0QXNAdDbRUORzEv4cz+8oZ3syTfGvSArkbVKsULrrX
haOLbb/lb/qt2igduLBEa2Ir/3WYpMw5Ead11FH57828QKzCywtdS3ZYKf4PuBRT
3T1N1NI1P+9oMQC860TPa7xzFVBIl3kvsUh6J7QurG5UnuGR0AznAMRiDC+YoW9m
588ZX/Z5nlYFiW3VIDvwoErWWf/3EvcZrGAbqvOoAVFEnlQ2eKdSFEHrwkkhF8Zg
St/O3ZEP+IzBzlr8stxVLxXSN81URHHlB4f3SLUtVZ45HHwR4WlMDk960SYwRzym
2mst91cS5fyEeCPkTvBkbhPQaVnYUBmYVqq1VN3v/9Y7W5DpKIxhIYBa4wXGSJ1I
2jaYXhxskNvbiiPogfRBWFZUvnx2oAMTUa0+3IXp05MlPh/1r1swv30KRyzRscjE
pLQVJo6s97pM1sTvd2fBWJnWCPYm3/dZSqNHPZt8BjmRvotXOhD29zb6FPaYJCs9
nyKR78VZVC9vPycFdWiUQYG2dG7GMcxL5ruDvEyqkU5YL8zAYZ8=
=WBwZ
-----END PGP SIGNATURE-----

On Sat, 21 Apr 2018 17:56:27 -0400
"Helmut K. C. Tessarek" <tessarek@evermeet.cx> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> 
> On 2018-04-21 17:23, wm4 wrote:
> > Just by the way, some have lamented that they think of it as
> > "doxing" when you post my real name on this list.  
> 
> I don't really care, if somebody is using a pseudonym or their real name.

Well I'm just saying how others interpreted CE's behavior.

> However, I have to say that if sombody is using not their real name,
> especially when their tone is often agressive and unfiltered, it
> rather begs the question why that is.
> 
> Is that person afraid that an employer might come across some of this
> crap when doing research on a potential candidate?
> Is that person not confident enough to stand by their opinion.
> 
> Seriously, what is it?
> 
> Btw, I myself often use blunt comments, which are mostly seen and read
> as harsh, undiplomatic, and rude. I hate poltical correctness and love
> sarcasm. Non of these traits are a plus in public. People love sugar
> coating and talking around a problem, all in the name of diplomacy.

It would be a big bother to change everything. Often you need a
pseudonym anyway, because account names are handier when they are
short. So why throw 2 names around?

Also this email address was meant as spam sink in the first place so it
has no meaningful name (registering to dozens of sites and mailing
lists which all show the mail address in plain text, which meant it'd
likely get a lot of spam). And one thing I don't want to do for sure is
giving Google my name for free. So I'd probably want to change my email
and email service too if I were to use my real name on this list.

Regarding employers, they'd get my "pseudonym" anyway because I'd put
a link to my github repository on my resume.

> 在 2018年4月22日，上午5:23，wm4 <nfxjfg@googlemail.com> 写道：
> 
> On Sat, 21 Apr 2018 22:55:33 +0200
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 
>> 2018-04-19 4:45 GMT+02:00, Steven Liu <lq@chinaffmpeg.org>:
>>> 
>>> 
>>>> On 19 Apr 2018, at 03:20, wm4 <nfxjfg@googlemail.com> wrote:
>>>> 
>>>> On Wed, 18 Apr 2018 16:10:26 -0300
>>>> James Almer <jamrial@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@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On Sun, 22 Apr 2018 08:41:18 +0800
Liu Steven <lq@chinaffmpeg.org> wrote:

> > 在 2018年4月22日，上午5:23，wm4 <nfxjfg@googlemail.com> 写道：
> > 
> > On Sat, 21 Apr 2018 22:55:33 +0200
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >   
> >> 2018-04-19 4:45 GMT+02:00, Steven Liu <lq@chinaffmpeg.org>:  
> >>> 
> >>>   
> >>>> On 19 Apr 2018, at 03:20, wm4 <nfxjfg@googlemail.com> wrote:
> >>>> 
> >>>> On Wed, 18 Apr 2018 16:10:26 -0300
> >>>> James Almer <jamrial@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@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?

You read it carefully, maybe apply the thing locally, and build and
test it. Reviewing basically means checking a patch for errors. Point
out the errors by replying to the patch, and ask the patch author to
fix them.

The more you can trust the patch author the more checks you can skip,
but generally you should at least skim the patch for things that look
wrong. In this case there were a bunch of obviously suspicious things,
including things we don't normally do (like the missing memory
allocation failure check). Looking whether a patch causes new compiler
warnings is also a thing worth to do.

And yes, this can be significant work, but it's all so to make sure bugs
are avoided, and that nobody has to fix the mess later.