[FFmpeg-devel] [PATCH] forbid strcpy

[Reimar Döffinger]

On Sun, Jan 31, 2010 at 02:10:21AM +0100, Michael Niedermayer wrote:
> >  4 files changed, 17 insertions(+), 11 deletions(-)
> > cedef4c87dca62be182df21287efa57239545e52  strcpy.diff
> > Index: libavformat/rdt.c
> > ===================================================================
> > --- libavformat/rdt.c	(revision 21548)
> > +++ libavformat/rdt.c	(working copy)
> > @@ -97,6 +97,7 @@
> >  ff_rdt_calc_response_and_checksum(char response[41], char chksum[9],
> >                                    const char *challenge)
> >  {
> > +    static const uint8_t tail[] = "01d0a8e3";
> >      int ch_len = strlen (challenge), i;
> >      unsigned char zres[16],
> >          buf[64] = { 0xa1, 0xe9, 0x14, 0x9d, 0x0e, 0x6b, 0x3b, 0x59 };
> > @@ -124,7 +125,7 @@
> >      for (i=0;i<32;i++) response[i] = tolower(response[i]);
> >  
> >      /* add tail */
> > -    strcpy (response + 32, "01d0a8e3");
> > +    memcpy(response + 32, tail, sizeof(tail));
> 
> so memcpy is better than strcpy ?

In so far that it is less likely to be misused and getting rid of strcpy
seems like a good idea to avoid people taking short-cuts e.g. because
they are to lazy to keep track how much space there is in the buffer.
You can't do that with memcpy without it being obvious.
However, av_strlcat would be an option, too, but that doesn't really
fit well into how the code works right now, it looks to me like
it can't decide if it's handling a string or a byte array.

> > Index: libavformat/mp3.c
> > ===================================================================
> > --- libavformat/mp3.c	(revision 21548)
> > +++ libavformat/mp3.c	(working copy)
> > @@ -325,15 +325,15 @@
> >          }
> >  
> >          if (!tag) { /* unknown tag, write as TXXX frame */
> > -            int   len = strlen(t->key), len1 = strlen(t->value);
> > -            char *buf = av_malloc(len + len1 + 2);
> > +            int   len = strlen(t->key) + strlen(t->value);
> > +            char *buf = av_malloc(len);
> 
> 2 bytes less?
> ill retry reviewing when you tested this

You know, the idea was that the maintainers look at it and either tell me
to continue to try to get rid of strcpy or not and ideally fix it themselves.
I value my time, too, and I can't be bothered to find samples that trigger
all of that code, so if everyone feels like they can't be bothered we'll just
leave it.