[FFmpeg-devel] [PATCH] forbid strcpy

Michael Niedermayer michaelni
Sun Jan 31 02:39:47 CET 2010


On Sun, Jan 31, 2010 at 02:29:35AM +0100, Reimar D?ffinger wrote:
> 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.

yes, its obvious above that you arent checking the buffer size at all ;)
that said greping for strcpy is easier than for "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.

If there are security issues i certainly will try to look at them but
my todo list is somewhat too full for a "replace all strcpy"

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100131/1e043f38/attachment.pgp>



More information about the ffmpeg-devel mailing list