[FFmpeg-devel] [PATCH] forbid strcpy

Michael Niedermayer michaelni
Sun Jan 31 02:10:21 CET 2010


On Sat, Jan 30, 2010 at 09:24:06PM +0100, Reimar D?ffinger wrote:
> On Sat, Jan 30, 2010 at 07:18:16PM +0000, M?ns Rullg?rd wrote:
> > "Ronald S. Bultje" <rsbultje at gmail.com> writes:
> > >> Please show me a single use where we'd notice if it was 20 times
> > >> faster.
> > >> I find it unlikely that there are cases where speed matters and
> > >> strcpy would be acceptable, it certainly is not particularly fast.
> > >
> > > I agree it won't make much of a difference, just expressing my feeling
> > > that we're entering a gray area.
> > 
> > Given the potential consequences of a badly applied strcpy(), it think
> > it's reasonable to ban it for general use.  If it really matters in
> > some specific case, it can be double-checked and allowed there.
> 
> I haven't looked at all uses yet, but I think I already agree more that I
> expected.
> Summary: only one place could even remotely be considered performance
> relevant, several others have at least needlessly code and introduce a
> high risk of introducing an exploit by simply increasing the size of some
> buffer (buffers declared with fixed size, not a define, or worse only
> one of them using a define for the size).
> E.g. look at libavformat/rtsp.c:rtsp_read_header
> I can't comment much on the whole filename handling, even though it seems
> suspicious to me, but this code combination:
>     av_strlcpy(reply->real_challenge, p, sizeof(reply->real_challenge));
>     strcpy(real_challenge, reply->real_challenge);
> means someone changing the size of the context variable without changing
> the other one has just created a on-stack buffer overflow.
> The situation is similar in libavutil/log.c, though I can admit some
> performance consideration there.
> The one in http.c is the same issue, except that the two relevant variables
> are not even close to each other, and one of them uses a define giving
> the illusion of being safely adjustable.
> The on in mp3.c is lacking a integer overflow check to be safe as-is (I first
> thought an overflow would not be possible, but it is for 64 bit systems).
> The one in avio.c would have been safe if it wasn't for av_mallocz taking
> an int as argument (but to be honest, I don't think whoever wrote it
> really even thought about it - I guess we probably have enough other place
> that limit the filename size so it probably isn't an issue - until some
> "fixes" those).
> Untested and otherwise improvable patch for some of those attached.

>  avio.c |   11 ++++++++---
>  http.c |    2 +-
>  mp3.c  |   12 ++++++------
>  rdt.c  |    3 ++-
>  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 ?


>  
>      /* calculate checksum */
>      for (i = 0; i < 8; i++)
> Index: libavformat/http.c
> ===================================================================
> --- libavformat/http.c	(revision 21548)
> +++ libavformat/http.c	(working copy)
> @@ -211,7 +211,7 @@
>          while (isspace(*p))
>              p++;
>          if (!strcmp(tag, "Location")) {
> -            strcpy(s->location, p);
> +            av_strlcpy(s->location, p, sizeof(s->location));
>              *new_location = 1;
>          } else if (!strcmp (tag, "Content-Length") && s->filesize == -1) {
>              s->filesize = atoll(p);
> 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

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

The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"
-------------- 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/945cde53/attachment.pgp>



More information about the ffmpeg-devel mailing list