22 ms·

Designing a better strcpy

• zabzonk 5y ago
  Referring to strcpy: > we can only use it if we know our destination buffer is smaller than our source buffer Should be the other way around, surely?
  • Randor 5y ago
    Absolutely, That's not the only discrepancy. https://pubs.opengroup.org/onlinepubs/9699919799/functions/memccpy.html https://pubs.opengroup.org/onlinepubs/9699919799/functions/m... Note the following: "The memccpy() function does not check for the overflow of the receiving memory area." "If copying takes place between objects that overlap, the behavior is undefined." The strxcpy he provides at the bottom doesn't look better at all. I'm not sure where the author got that function. I found some better variants of the proposed strxcpy function with bounds checks and that provides overflow detection.
    • saagarjha 5y ago
      I wrote strxcpy as an example of a function that satisfies the requirements that I posted at the top. In doing so, it becomes a useful routine for other kinds of needs, such as this one: https://news.ycombinator.com/item?id=27564004 https://news.ycombinator.com/item?id=27564004 The reason I omitted the specific verbiage you quoted is that they apply equally to all the functions. My function has an important bounds check, but (like all of C) trusts that the parameters you provide to it are correct. There is no additional checking done because doing so in C is not tenable. If implemented in a standard library it may be useful to add additional heuristics to detect invalid cases, but they are fundamentally best-effort and not worth showing in an example.
  • saagarjha 5y ago
    LOL yes, thanks for catching that. I’ll fix it when I’m at my computer :) Edit: edited. I also had a messaging from past me that I had forgotten about: <!-- I made a mistake somewhere, didn't I? -->
• vasama 5y ago
  Migrating to a pointer-size-pair string representation would be a better use of one's time.
  • Gibbon1 5y ago
    I swear part of the problem is with C there is a cargo cult prohibition against passing small structs by value.
    • kzrdude 5y ago
      In other languages we create types for just about anything, and like you say, it's strange that we don't do this more in C.
      • Gibbon1 5y ago
        Big problem with C is the standards committee just flat out refuses to add an array type to C. It's deranged because if you had first class arrays it'd be a lot easier to generate code that takes advantage of SIMD instructions.
        • klodolph 5y ago
          You mean “add a second array type?” C has an array type, it’s just a bit wacky, and it doesn't play by the same rules as other types.
          • pjmlp 5y ago
            Yeah because requiring &array[0] instead of array is so much typing, UNIX V4 would never have been finished.
            • klodolph 5y ago
              You want to break existing code? If you made that change, you wouldn’t even be able to pass string constants into functions, because string constants are arrays (and not pointers, as some think). The C standard committee has a general policy of not breaking code. If you want something like C with a better type system and less of the broken stuff (hello, arithmetic conversions) you can try Zig or one of the others. Once you’re okay with breaking code, you call it a different language, and go in trying to fix lots of things. Unix V4 predates C standardization anyway.
• deleted 5y ago
  [deleted]
• WalterBright 5y ago
  An even better solution: https://www.digitalmars.com/articles/C-biggest-mistake.html https://www.digitalmars.com/articles/C-biggest-mistake.html With the lengths of strings known, copying becomes fast, safe, and trivial.
  • Hackbraten 5y ago
    Not everyone is going to be happy about 8 bytes overhead for every buffer. Also, not every C-style API will migrate. If you consume one of those, you may still have to count the length, which costs cycles.
    • WalterBright 5y ago
      The proposal does not take away any C features.
    • scottlamb 5y ago
      > Not everyone is going to be happy about 8 bytes overhead for every buffer. Then prefix it with four bytes (which, if you omit the trailing NUL, adds three bytes in total). If your C strings are longer than 2^32 bytes, you're probably doing something wrong. I also should point out that memory is typically cheap. Cache is more precious, and I'd expect on balance adding a length would pollute the cache less than scanning through the whole string unnecessarily. But I agree with your second point: > Also, not every C-style API will migrate. If you consume one of those, you may still have to count the length, which costs cycles. Similarly, the article started with this text: > Like them or not, null-terminated strings are essential to C, and working with them is necessary in all but the most trivial programs. You can do things nicely in your own code, but you still need NUL-terminated strings when dealing with existing code. And dealing with existing code is often the reason to pick C...
      • iib 5y ago
        I would assume the people bothered by an increase in byte-sized overhead are not the ones that are using 2^32 length strings. About the cache, I think for small strings, scanning the whole string may still be faster. Though adding a four-byte length prefix would make certain functions like strlen almost go away.
• saurik 5y ago
  > In the case where src fits in dst, it will return a pointer past the NUL byte it placed; otherwise it returns NULL to indicate a truncation. It is amazing to me how personal these preferences are ;P... like, I'd be much happier with an API that always returns the location of the NUL byte on success; and, if the string gets truncated, then it instead returns dst+len (the address of the byte past the end of the buffer). This allows for chained constructions that provide efficient strcat-style semantics with easy error propagation, such as this example which concatenates three strings (which I honestly hope I got right... I'm giving reasonable odds to Saagar telling me I've coded a buffer overflow by accident somewhere ;P): char buf[X]; // for any X, even 0! char *cur = buf; char *end = buf + sizeof(buf); cur = strjcpy(cur, str1, end - cur); cur = strjcpy(cur, str2, end - cur); cur = strjcpy(cur, str3, end - cur); if (cur == end) goto fail;
  • kolbusa 5y ago
    Did you mean to check for (cur > end)?
    • saurik 5y ago
      No: if the function succeeds, the address of the NUL byte will always be !=end (as it must be inside of the buffer, which end is not); whereas, in the case of string truncation, cur will be equal to end, as the function returns dst+len (which is end); and, if that error had "already" happened during a previous call, it will get propagated through the next call as end-cur will be 0, causing the next call to immediately fail (even for a 0-length string, which is an important corner-case) and return cur+0 (which is still end).
  • saagarjha 5y ago
    Not seeing anything wrong, but it’s C so who knows ;) Thankfully, the primitive I (and memccpy) provide makes writing your wrapper easy and efficient, as opposed to all the other functions which don’t compose at all. (From my phone) I think this might work? char *strjcpy(char *dst, const char *src, size_t len) { char *result = strxcpy(dst, src, len); return result ? --result : result + len; }
• Tempest1981 5y ago
  Something feels wrong about using memccpy to copy strings... ever since I saw bugs where people used memcpy incorrectly. And is there a wchar_t version of memccpy?
  • saagarjha 5y ago
    memccpy≠memcpy, even though their names are similar. While the function is somewhat generic, I would bet 90+% of its use is going to be to copy null-terminated strings.
    • Tempest1981 5y ago
      Right, maybe I'm just reacting to the name. I've had to educate many novices about using memcpy with incorrect lengths... either 1 short (no null), or even sizeof(src) and wondering why only 4 or 8 bytes got copied. Now if I steer them to hot new memccpy, they may hear "memcpy" and run off: "memcpy? Sure, I know that!"
• stefan_ 5y ago
  I don't want to use any of the str*cpy functions, all of them are either braindead or missing in most libcs. At this point I'm all in on snprintf(%s, foo).
  • saagarjha 5y ago
    I hate to do this, but: read the post. I mention snprintf specifically: https://saagarjha.com/blog/2020/04/12/designing-a-better-strcpy#snprintf https://saagarjha.com/blog/2020/04/12/designing-a-better-str.... It is neither efficient nor general-purpose as a string copying routine.
• rbanffy 5y ago
  Would it make sense to extend the C standard to have a sensible string type? /me ducks
  • pjmlp 5y ago
    It would, just like a sensible array type and proper enumerations as well. Obviously WG14 doesn't care about it.
• okareaman 5y ago
  I expected to read about SIMD and copying 8 bytes at a time in 64 bit registers, but I guess that is a compiler optimization
• tedunangst 5y ago
  The concern with performance seems a little overwrought. strlcpy is only slow in the bad case where it truncates, which is ideally not the common case. I've never heard or seen of a performance bottleneck traced to a strlcpy in the hot path. If you really cared about performance, you'd be using nothing but memcpy with careful length tracking. Regardless of algorithmic runtime, any function that examines bytes as it copies will be slower than a length based copy.
  • saagarjha 5y ago
    I think one use for this kind of thing is if you are doing some sort of logging you may have a fixed size buffer, both to keep overhead down (you don't want to allocate extra memory) and also prevent overly verbose logs from spamming output. In this case, waiting to calculate the length of a 10 MB string just so you can fit it in a 1 KB buffer is unacceptable. For your second point: not necessarily! memcpy would be slightly faster if you are keeping the length around, but if you're dealing with arbitrary strings you wouldn't have that. Calculating the full length beforehand is just a no-go as I mentioned above, and using memchr first to get the length and then memcpy is not going to be faster.
    • ec109685 5y ago
      Example where etcd’s logger was calculating log size and suffering from a performance issue as a result: https://github.com/etcd-io/etcd/issues/12835 https://github.com/etcd-io/etcd/issues/12835
    • tedunangst 5y ago
      Use snprintf with a precision specifier.
• dataflow 5y ago
  I hate to nitpick on something so mundane and superficial, but why in the world are people still writing code like this in 2020? while (--len && (*dst++ = *src++)) ; Dereferenced post-increments are already confusing enough as-is, and yet here we have 1 pre-increment and 2 dereferenced post-increments happening on top of an assignment in a conditional, all in a single expression. Even as someone who does put an assignment in a conditional once in a while, this still feels 100% unjustifiable to me. It's especially ironic given the premise is that C code has security bugs... if the goal is to avoid that, shouldn't there be even more care taken to avoid this kind of code? EDIT: For those who don't think the readability can be improved... any thoughts on something more like this? Do we really need a compound assignment with three side effects in a conditional modifying the function arguments to make this readable? char *strxcpy(char *restrict dst, const char *restrict src, size_t len) { size_t i; for (i = 0; i < len; ++i) { if (src[i] == '\0') { break; } dst[i] = src[i]; } int input_exhausted = i < len; if (input_exhausted) { ++i; } if (i > 0) { dst[i - 1] = '\0'; } return input_exhausted ? &dst[i] : NULL; }
  • Tempest1981 5y ago
    Homage to K&R? Hard to unlearn that style. How about reading the STL source code? I know it's not deliberately obfuscated, but sometimes feels that way.
    • mhh__ 5y ago
      A lot of it is deliberate because they have to avoid name collisions thanks to C++ header files being incredibly stupid
  • abhishekjha 5y ago
    I guess this is so that something like this can be asked in interviews to test how lucky the candidate is on that particular day and hour. Otherwise everybody would pass the interviews.
  • FabHK 5y ago
    I don't know, it could be written more verbosely, but then, this is just so idiomatic. Not sure writing it out makes it much more legible. ETA: yes, your version reads really well. Question though: does it require an extra multiplication and addition for every "[i]" in the code below, or are compilers smart enough to optimise that these days? It seems to me that for sufficiently dumb compilers, your "nice" version would be slower. for (i = 0; i < len; ++i) { if (src[i] == '\0') { break; } dst[i] = src[i]; }
• deleted 5y ago
  [deleted]
• notaplumber 5y ago
  > strlcpy... it’s not standard so it doesn’t satisfy 5 either. It's a de facto standard supported on *BSD/macOS/Android/Linux(w/ musl libc) and a number of other commercial systems, it's also on track for standardization in POSIX (Issue 8). https://www.austingroupbugs.net/view.php?id=986#c5050 https://www.austingroupbugs.net/view.php?id=986#c5050 https://www.opengroup.org/austin/docs/austin_1110.pdf https://www.opengroup.org/austin/docs/austin_1110.pdf While it might not meet your arbitrary set of requirements, it already has decent adoption.
  • saagarjha 5y ago
    Yikes, I hope they don’t add it, that would make people want to use it even more. Given its surprising performance characteristics it’s usually not what you want.
    • notaplumber 5y ago
      It's looking very much like it will, but even if that wasn't the case, it's already widely in use and available in OS libc's and easy to copy, and it has been since OpenBSD first introduced it over 20 years ago. I also believe your performance claims are exaggerated.. and strlcpy does exactly what people want and expect in most scenarios.
      • deleted 5y ago
        [deleted]
      • saagarjha 5y ago
        I've seen a lot of strlcpys, and of that I think I have seen maybe one place where the return value was used. I think if you asked people why they used strlcpy, 95+% would say "security" and then list out the characteristics my strxcpy has, rather than "I want snprintf-like behavior with a size_t return" which is what strlcpy is. I would not be surprised if most people got the time complexity of the function incorrect because of this.
        • 5y ago
• wruza 5y ago
  Rant ahead. (which value to return) This always bugged me in C interfaces. All these metrics are accumulated in the scope of your qweasd(), why not at least take a pointer to a “struct strcpy_result” instead and return them all with it, dammit, stack space is almost always free. After two decades of C, I turned to jitted scripting and hopefully will NEVER look back (but thanks for the xp). Low-level bit-fiddle style programming outside of very restrictive embedded tech is a distilled schizophrenia of tiling sharp irregular shapes, trying, failing, vapid engineering, byte economy, and finally losing a larger part of the ephemeral benefits that you never needed (or were able to get) in the first place. (C++ being a drug that doesn’t fix the issue, but at least reduces voices in your head to “just” a heavy form of OCD). discussion on losing 4/8 bytes per string in this thread Insanity.
• towergratis 5y ago
  I would use ssize_t instead of size_t. Mixing unsigned and signed is seriously broken in C, and hence better to stick with signed.
  • minipci1321 5y ago
    > Mixing unsigned and signed is seriously broken in C, and hence better to stick with signed. Mmmm .. maybe in this specific case (didn't look). But if you meant this as general advice, then one should keep in mind that unsigned overflows are specified but signed overflow is UB (barring maybe the very latest version of C standard); because of that, unsigned division in many cases is trivially optimised to less complex ops, etc. There are many advantages in using unsigned.
• zbuf 5y ago
  > While string truncation has its own issues, it is often a fairly reasonable fallback. Seemingly swimming against the tide here; a strcpy() that _automatically_ truncates strings is a worse, and huge hidden risk. Yes, buffer overflows from user data are very, very bad. But a buffer overflow has a clear fingerprint as leaning on undefined behaviour; tools exist to detect these (eg. valgrind), and the fix is well understood. Defining strcpy to include truncation is the wrong choice, as it's plain dangerous in most cases. It reclassifies the bug as valid program behaviour, where it's riskier and its fingerprint is harder to detect. For example, a function which looks up credentials for a login name limited to 16 characters. During lookup, a buffer truncates and by default the code is now doing lookups against the wrong login name. The better solution is a strcpy() which accepts a length and is _expressly not defined_ for cases where the output buffer would overflow; ie. asserts or aborts (which now has scope to be detected at compile time for some cases) It's now clear where bug is, and a developer must preempt overflow for cases where it's possible and handle it. Perhaps folks have different experiences, but mine is that copying a string and _wanting_ to truncate it is so incredibly rare and is the cases that should be explicit, not implicit.
  • saagarjha 5y ago
    Failing perceptibly composes better than failing loudly. It's easy to go one way: char *strxxxcpy(char *restrict dst, const char *restrict src, size_t len) { char *result = strxcpy(dst, src, len); assert(result); return result; } but going in reverse is difficult.
    • zbuf 5y ago
      But that misses my point; the bug is that as a developer it's too easy to not write the correct check -- like exactly this one. The library function presented to most developers (especially a novice one) should reflect the most common case, not the more-easy-to-compose case.