my version of strlcpy

Question

gcc 4.4.4 c89

My program does a lot of string coping. I don't want to use the strncpy as it doesn't nul terminate. And I can't use strlcpy as its not portable.

Just a few questions. How can I put my function through its paces to ensure that it is completely safe and stable. Unit testing?

Is this good enough for production?

size_t s_strlcpy(char *dest, const char *src, const size_t len)
{
    size_t i = 0;

    /* Always copy 1 less then the destination to make room for the nul */
    for(i = 0; i < len - 1; i++)
    {
        /* only copy up to the first nul is reached */
        if(*src != '\0') {
            *dest++ = *src++;
        }
        else {
            break;
        }
    }

    /* nul terminate the string */
    *dest = '\0';

    /* Return the number of bytes copied */
    return i;
}

Many thanks for any suggestions,

Answer 1

Although you could simply use another strlcpy function as another post recommends, or use snprintf(dest, len, "%s", src) (which always terminates the buffer), here are the things I noticed looking at your code:

size_t s_strlcpy(char *dest, const char *src, const size_t len)
{
    size_t i = 0;

No need to make len const here, but it can be helpful since it checks to make sure you didn't modify it.

    /* Always copy 1 less then the destination to make room for the nul */
    for(i = 0; i < len - 1; i++)
    {

Oops. What if len is 0? size_t is usually unsigned, so (size_t)0 - 1 will end up becoming something like 4294967295, causing your routine to careen through your program's memory and crash into an unmapped page.

        /* only copy up to the first nul is reached */
        if(*src != '\0') {
            *dest++ = *src++;
        }
        else {
            break;
        }
    }

    /* nul terminate the string */
    *dest = '\0';

The above code looks fine to me.

    /* Return the number of bytes copied */
    return i;
}

According to Wikipedia, strlcpy returns strlen(src) (the actual length of the string), not the number of bytes copied. Hence, you need to keep counting the characters in src until you hit '\0', even if it exceeds len.

Also, if your for loop terminates on the len - 1 condition, your function will return len-1, not len like you'd expect it to.

---

When I write functions like this, I usually prefer to use a start pointer (call it S) and end pointer (call it E). S points to the first character, while E points to one character after the last character (which makes it so E - S is the length of the string). Although this technique may seem ugly and obscure, I've found it to be fairly robust.

Here's an over-commented version of how I would write strlcpy:

size_t s_strlcpy(char *dest, const char *src, size_t len)
{
    char *d = dest;
    char *e = dest + len; /* end of destination buffer */
    const char *s = src;

    /* Insert characters into the destination buffer
       until we reach the end of the source string
       or the end of the destination buffer, whichever
       comes first. */
    while (*s != '\0' && d < e)
        *d++ = *s++;

    /* Terminate the destination buffer, being wary of the fact
       that len might be zero. */
    if (d < e)        // If the destination buffer still has room.
        *d = 0;
    else if (len > 0) // We ran out of room, so zero out the last char
                      // (if the destination buffer has any items at all).
        d[-1] = 0;

    /* Advance to the end of the source string. */
    while (*s != '\0')
        s++;

    /* Return the number of characters
       between *src and *s,
       including *src but not including *s . 
       This is the length of the source string. */
    return s - src;
}

Answer 2

IMHO, just barrow the original strlcpy, which Ignacio Vazquez-Abram tersely stated. OpenBSDs code is battletested and the license terms rock ;).

As to your code, something I would add to what has already been said by others, is just a matter of personal taste:

/* only copy up to the first nul is reached */
if(*src != '\0') {
    *dest++ = *src++;
}
else {
    break;
}

I would have written this like this:

if(*src == '\0') {
    break;
}
*dest++ = *src++;

Both because it reduces on the amount of unnecessary code people need to read, and because it is my 'style' to consistently write that way, instead of if (ok) { do } else { handle error }. The comment above the if is also redundant (see comment above the for loop).