Date: Fri, 26 Dec 1997 10:45:24 -0800 (PST)
From: Dean Gaudet <dgaudet@arctic.org>
To: new-httpd@apache.org
Subject: Re: [PATCH] ap_cpystrn() function (replace strncpy)
In-Reply-To: <199712261651.LAA09735@devsys.jaguNET.com>
Message-ID: <Pine.LNX.3.95dg3.971226103308.8918A-100000@twinlark.arctic.org>
Organization: Transmeta Corp.
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: new-httpd-owner@apache.org
Precedence: bulk
Reply-To: new-httpd@apache.org


On Fri, 26 Dec 1997, Jim Jagielski wrote:

> Index: src/main/http_config.c
> ===================================================================
> RCS file: /export/home/cvs/apachen/src/main/http_config.c,v
> retrieving revision 1.89
> diff -u -r1.89 http_config.c
> --- http_config.c	1997/12/16 07:36:30	1.89
> +++ http_config.c	1997/12/26 16:41:48
> @@ -1134,8 +1134,7 @@
>      /* Global virtual host hash bucket pointers.  Init to null. */
>      init_vhost_config(p);
>  
> -    strncpy(coredump_dir, server_root, sizeof(coredump_dir) - 1);
> -    coredump_dir[sizeof(coredump_dir) - 1] = '\0';
> +    ap_cpystrn(coredump_dir, server_root, sizeof(coredump_dir) - 1);
>  }

The length are should be just sizeof(coredump_dir), without the -1. 
There's a few more cases of it too.  It's probably easiest for you to take
your patch file, and edit it to remove the -1s and the apply that to a
fresh tree. 

> +char *ap_cpystrn(char *dst, const char *src, size_t len)
> +{
> +
> +    char *d = dst;
> +    char *end = dst + len;
> +
> +    while (len && (*d++ = *src++))
> +        len--;
> +
> +    if (d >= end)
> +        *end = '\0';	/* always NULL terminate */
> +
> +    return dst;
> +}

len is an induction variable, but compilers may have a hard time detecting
it.  We can speed up the loop like this:

char *ap_cpystrn(char *dst, const char *src, size_t len)
{
    char *d = dst;
    char *end;

    if (len == 0) {
	/* there's nothing we can do */
	return (dst);
    }
    /* save one byte for termination */
    end = dst + len - 1;
    while (d < end && (*d++ = *src++)) {
	/* nop */
    }
    *d = '\0';
    return dst;
}

One more thing I'd like to bring up is that I think doing return(d) at the
end is far more useful.  It won't tell you definately that truncation occured,
there's the case where the string to be copied is exactly len-1.  But it is
more useful in general should we ever want to use ap_cpystrn to fill in an
array bit by bit:

    char *d;
    char *buf_end;
    char buf[nnn];

    d = buf;
    buf_end = buf + sizeof(buf);
    for (something) {
	...;
	d = ap_cpystrn(d, something, buf_end - d);
	if (d == buf_end - 1) {
	    error, probably truncation, report it as truncation anyway;
	}
	...;
    }

Dean