4
\$\begingroup\$

strscpy() is similar to the standard strncpy() except that it always writes a valid null-terminated string (unless size is zero).

So, I have two questions here:

  1. Is my basic implementation of strscpy() correct?
  2. Is using <errno.h> constant macros as negative return values something that should be avoided in general?
#include <errno.h>

ssize_t ft_strscpy(char *dst, const char *src, size_t size)
{
    size_t  cnt;

    if (!size)
        return (-E2BIG);
    cnt = 0;
    while (*src && cnt < size - 1)
    {
        *dst++ = *src++;
        ++cnt;
    }
    *dst = '\0';
    if (*src)
        return (-E2BIG);
    return (cnt);
}

I wanted to write my own strscpy() to replicate the behavior of the one used in Linux kernel.

The source code is quite complex so I didn't understand most of the optimizations there but I noticed the function returns -E2BIG which expands to -7 instead of the usual -1 I'm used to see on error returns.

How do you feel about this?

When I saw this I immediately thought about replicating this idea in my own code to have more meaningful returns than a generic -1.
But then, I started thinking maybe it's not the best idea, and there might be reasons why I haven't seen this done anywhere else before..

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Can you include how strcpy() differs from strscpy()? \$\endgroup\$
    – Harith
    Commented May 22 at 15:40
  • \$\begingroup\$ Should I include this information in my original post? Here is the Debian man page for strscpy() manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html \$\endgroup\$
    – ismbks
    Commented May 22 at 15:45
  • 2
    \$\begingroup\$ Better if you add the description to the post, as it is a function unheard of. At least I hadn't seen it before. \$\endgroup\$
    – Harith
    Commented May 22 at 15:50
  • \$\begingroup\$ Why would you do this? ssize_t is awkward in several ways and this really only belongs in kernel mode and other places where you don't have a global errno. \$\endgroup\$
    – Joshua
    Commented May 23 at 20:13

3 Answers 3

3
\$\begingroup\$

Your implementation is correct.

I dislike the use of a negative return value in this situation. What is the problem that the function is trying to fix? I think that a large part of the problem is incorrect invocation, so I think that many invocations will ignore the return value and those that don't will mysteriously go wrong when it's the error value. I would prefer a design like this:

char *strxcpy(char *dst, const char *src, size_t size)
{
 char *enddst = dst + size;
 if (dst == enddst) return 0;
 do {   // there is space for at least one char at dst
        if (! (*dst = *src++) ) return dst;
 } while (++dst < enddst);
 dst[-1] = '\0';   // run out of space - terminate what we copied
 return 0;
}

By returning a pointer, we ensure that any use of the return value without checking for an error will very likely be caught immediately, rather than causing some hard to find bug whereby something has had a bogus amount added to it. Some static code checkers may even be able to tell that the caller is missing a null pointer check.

Also, I prefer buffers to be described by a start and end pointer - not a start and length. As you can see, this eliminates a bug-prone need to keep dst and cnt in step (pretty trivial in this very simple example, but harder to guarantee in general). While you specified the parameters, so I calculated enddst, it might be advantageous to change the specification so as to pass it in. This means that you can write a sequence like this:

    p = strxcpy(buf, "hello", endbuf);
    p = strxcpy(p, ", ", endbuf);
    p = strxcpy(p, "world", endbuf);

and each step is simple and clearly passing in the correct thing. Note what happens with an error return - we get a null pointer exception. Error checking this means saying:

    p = strxcpy(buf, "hello", endbuf);
    if (p) p = strxcpy(p, ", ", endbuf);
    if (p) p = strxcpy(p, "world", endbuf);
    if (!p) /// handle buffer too small

Still very simple. Whereas:

    pos = strscpy(buf, "hello", sizeof(buf));
    pos += strscpy(buf + pos, ", ", sizeof(buf) - pos);
    pos += strscpy(buf + pos, "world", sizeof(buf) - pos);

is more complicated and harder to verify as you need to check that the thing being added to buf for the first parameter is the same thing being subtracted from the size for the third. Now unnoticed failure causes us to step back 7 (E2BIG is 7) chars in the buffer, which either makes us start re-writing a part of it or, much worse, makes us write before the start of the buffer. Adding error checking is more complicated, because we need to keep the old value of pos until the return value is tested.

    r = strscpy(buf, "hello", sizeof(buf));
    if (r >= 0) {
        pos = r;
        r = strscpy(buf + pos, ", ", sizeof(buf) - pos);
    }
    if (r >= 0) {
        pos += r;
        r = strscpy(buf + pos, "world", sizeof(buf) - pos);
    }
    if (r >= 0) pos += r;
    if (r < 0) // handle error

Some people may prefer to write this with nested if statements marching off to the right.

If buf is a pointer rather than an array, the difference is even bigger:

    r = strscpy(buf, "hello", buflen);
    buf += r; buflen -= r;
    r = strscpy(buf, ", ", buflen);
    buf += r buflen -= r;
    r += strscpy(buf, "world", buflen);
    buf += r; buflen -= r;

and adding error checking to this is left as an exercise for the reader.

\$\endgroup\$
2
  • \$\begingroup\$ Because I then proceed to explain why I think the length should not be passed in, so it makes this version as close as possible to one which gets enddst as a parameter. If you think it always returns a null pointer, I suggest you try compiling it with a simple test case. \$\endgroup\$
    – c19
    Commented Jun 8 at 23:35
  • \$\begingroup\$ Yes I missed the if (! (*dst = *src++) ) return dst;, so code does not always return NULL. Comment removed. \$\endgroup\$ Commented Jun 9 at 0:32
7
\$\begingroup\$

Use C99's new meaning of static keyword:

If we shift around the parameters, we can have:

ssize_t ft_strscpy(size_t size,
                   char dst[restrict static size], 
                   const char src[restrict static size]);

And now if we call it like this:

int main(void)
{
    char buf[10] = {};
    ft_strscpy(10, buf, "feo");
}

The compiler outputs some diagnostic information nicely:

c.c: In function 'main':
c.c:26:5: warning: 'ft_strscpy' reading 5 bytes from a region of size 4 [-Wstringop-overread]
   26 |     ft_strscpy(5, buf, "feo");
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
c.c:26:5: note: referencing argument 3 of type 'const char[]'
c.c:3:9: note: in a call to function 'ft_strscpy'
    3 | ssize_t ft_strscpy(size_t size,
      |         ^~~~~~~~~~

It is also more self-documentary, and restrict might allow the compiler to better optimize the function as it can assume that the two pointers do not alias.

Missing unit-tests:

Some things we can test:

  • Does the function correctly return the number of bytes copied?
  • Does the function return -E2BIG if the destination buffer is not large enough?
  • Do the two strings compare equal after the function returns?

You do not require a testing library/framework for this. Use assert() from <assert.h>.


I noticed the function returns -E2BIG which expands to -7 instead of the usual -1 I'm used to see on error returns.

How do you feel about this?

How I feel is irrelevant if I am trying to implement strscpy(9) as per the specification here.

If you do not like returning -E2BIG, you can return something else, but then it would no longer be strscpy(), which is specified to return -E2BIG, but a modified version which you should specify in the documentation.

See: Why return a negative errno? (e.g. return -EIO).

Noise:

return doesn't require any parenthesis. Change this:

return (-E2BIG);
...
return (cnt);

to:

return -E2BIG;
...
return cnt;

Use meaningful identifiers:

cnt is better as count, at least it does not read and sound like vulgar slang.

Initialize variables at the point of declaration:

size_t  cnt;

if (!size)
    return (-E2BIG);
cnt = 0;

There is no reason for assignment cnt = 0 to be so far from its declaration. Consider doing:

size_t cnt = 0;

This is how I'd implement ft_strscpy():

#include <errno.h>
#include <string.h>

ssize_t ft_strscpy(size_t size,
                   char dst[restrict static size], 
                   const char src[restrict static size])
{
    if (size == 0) {
        return -E2BIG;
    }

    void *const res = memccpy(dst, src, '\0', size);
    return res != nullptr ? (res - dst - 1) : (dst[size - 1] = '\0', -E2BIG);
}

I've shifted the parameters around. If you wish to stay true to the specification, you should use the same prototype as the one in your code. And whilst others might frown at the use of the comma operator, I find it okay here.

For more information about memccpy(), see: memccpy(3) — Linux manual page. It is part of C23.

If you ask me, it renders strncpy(), strlcpy(), strscpy(), stpcpy(), strcpy_s(), strncpy_s(), and other variants almost useless. At least strncpy() can finally die now.

\$\endgroup\$
18
  • 1
    \$\begingroup\$ I didn't DV. but since the argument order is already specified in the original definition, I don't think it's appropriate to shift the parameters around. \$\endgroup\$
    – Barmar
    Commented May 23 at 14:45
  • 1
    \$\begingroup\$ AIUI, strncpy() was created for the benefit of the original Unix filesystem, which had a 14-character field for filenames, and allowed maximum-sized filenames to be non-null-terminated. AFAIK that filesystem is long obsolete, so indeed we don't really need strncpy() any more. \$\endgroup\$
    – Barmar
    Commented May 23 at 14:55
  • 1
    \$\begingroup\$ @Harith Ah, OK. MSVC does support that, through SAL. \$\endgroup\$
    – Davislor
    Commented Jun 6 at 4:23
  • 1
    \$\begingroup\$ @Davislor I wasn't saying that we don't need a version of strcpy() with a limit, I was saying it should be like strscpy(), not like strncpy(), so the result always has a null terminator. \$\endgroup\$
    – Barmar
    Commented Jun 6 at 15:05
  • 1
    \$\begingroup\$ @Davislor Right, C is generally designed to work only with null-terminated strings. As I said, this was created for a specific case where the null terminator was made optional to optimize disk space, so they didn't have to waste a byte for max-length filenames. There was a time when memory and disk space were expensive, and every byte mattered. \$\endgroup\$
    – Barmar
    Commented Jun 7 at 14:42
4
\$\begingroup\$

Opinion: argument order

ssize_t ft_strscpy(char *dst, const char *src, size_t size), even though it matches sized_strscpy(), separates dst and size. I'd expect the most common usage would be something like:

char dst[100];
const char src = ...;
ssize_t retval = ft_strscpy(dst, src, sizeof dst);

whereas

ssize_t retval = ft_strscpy(dst, sizeof dst, src);
// or 
ssize_t retval = ft_strscpy(sizeof dst, dst, src);

keeps a pointer to its destination and size as adjacent arguments.

Further, with

ssize_t retval = ft_strscpy(sizeof dst, dst, src);

we can code the below, somewhat like @Harith good answer.

ssize_t ft_strscpy(size_t size,
    char dst[restrict static size], 
    const char src[restrict 1]);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.