Page MenuHomeFreeBSD

strcpy(3): Explicitly mention the recommended usage of strlcpy()
Needs RevisionPublic

Authored by otis on Apr 13 2022, 8:41 AM.
Tags
None
Referenced Files
F80228546: D34896.diff
Fri, Mar 29, 11:02 AM
Unknown Object (File)
Jan 11 2024, 10:36 AM
Unknown Object (File)
Dec 29 2023, 5:04 PM
Unknown Object (File)
Dec 20 2023, 7:23 AM
Unknown Object (File)
Dec 15 2023, 4:36 AM
Unknown Object (File)
Nov 6 2023, 5:33 PM
Unknown Object (File)
Sep 15 2023, 12:02 PM
Unknown Object (File)
Jun 17 2023, 4:06 PM

Details

Reviewers
jmg
pauamma_gundo.com
Group Reviewers
manpages
Summary

Suggested by: jmg

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 45142
Build 42030: arc lint + arc unit

Event Timeline

otis requested review of this revision.Apr 13 2022, 8:41 AM

I'd prefer if you used .Sy or .Em rather than capitalization.

Playing devil's advocate, I can't help but not include (one of many) famous Ulrich's quotes:

Dammit, it is not safe. It hides bugs in programs. If a string is too long for an allocated memory block the copying must not simply silently stop. Instead the program must reallocate or signal an error. I can construct you cases where the use of these stupid functions is creating new security problem.

Admittedly, his canonical way to handle these situations uses unportable mempcpy() function, a GNU extension which returns adjusted destination, but still, all-caps scary warning might be a bit too much for the manpage, despite the almost modifier. IMHO it should be either toned down, or better elaborated.

I am curious who the intended recipient of this warning is -- I suspect that those who need to see it are unlikely to be reading the man page anyhow.

I am curious who the intended recipient of this warning is -- I suspect that those who need to see it are unlikely to be reading the man page anyhow.

Or dismiss the warning as currently worded, if they read the page.

lib/libc/string/strcpy.3
56–59

I find that wording (and specifically "admonishement") very patronizing. If I were about to use this function and reading this, I would almost certainly think "... Yeah, no. I know what I'm doing, thank-you-very-much." (Whether hypothetical!me actually would is irrelevant here.)

I'll second the usage of Em or Sy, I'm a bit nervous about Sy, being symbolic, and how it'll format when using a proportional font (e.g. printed).

Also, still missing from the security considerations section is that strncpy prevents data leakage by overwriting the entire buffer which strlcpy does not do.

Instead of this, I'd extend SECURITY CONSIDERATIONS with:

  • A naive/straightforward use of strcpy (I'm sure historical bad examples abound *side-eyes Sendmail Vulnerability Of The Week™ back in the 1990s and early 2000s*)
  • A discussion of the problem(s) with it
  • A suggested rewrite using strlcpy/strncpy/whatever
  • An explanation of the advantages of that rewrite (how it addresses the problems given) and trade-offs/shortcomings of the rewritten solution (someone mentioned memory leaks)

Then it's actually usable by people who need it, with 100% less unhelpful moralizing. For additional community service, suggest the change upstream.

This revision now requires changes to proceed.Apr 15 2022, 8:11 AM

Playing devil's advocate, I can't help but not include (one of many) famous Ulrich's quotes:

Dammit, it is not safe. It hides bugs in programs. If a string is too long for an allocated memory block the copying must not simply silently stop. Instead the program must reallocate or signal an error. I can construct you cases where the use of these stupid functions is creating new security problem.

Admittedly, his canonical way to handle these situations uses unportable mempcpy() function, a GNU extension which returns adjusted destination, but still, all-caps scary warning might be a bit too much for the manpage, despite the almost modifier. IMHO it should be either toned down, or better elaborated.

I've never heard or read the quote before, but I've always agreed with it.
If you want to detect truncation when it happens, you can just compare the size of the destination buffer with strlen() on the source.
If you want to use the possibly truncated result anyway, you can even use memcpy(d, s, n)[n - 1] = 0;.

For what it's worth, we do provide mempcpy, but we don't seem to provide strcpy_s.

@pstef wrote:

I've never heard or read the [Drepper's] quote before, but I've always agreed with it.

It easily comes up among the first results once you google the keywords of this saga. :-) There is somewhat nicer, less agitated discussion on LWN, here're some excerpts:

  • [Drepper's] reasoning against strlcpy() is relatively valid. It isn't actually safe. It protects against buffer overflows, but what you really want to do is check beforehand anyway and either error out or allocate a larger buffer, because truncated text in many contexts is just as much of a security issue as a buffer overrun, especially in cases where some kind of validation is done on the string before the copy is made;
  • Even if it's not a security issue, in most cases the user is far better served by an error or warning about the truncated text than she is by just having her data silently cut off. Advocating users to use strlcpy() is going to leave them just as lazy and will just shift software from one problem to another, instead of teaching them to Do It Right™ or even better just doing it right in the first place;
  • The strl* family is not in POSIX 2008. They were proposed but rejected since they did not really solve the problem (as pointed out above);
  • The OpenBSD's strlcat() has different behavior than the Solaris' strlcat(), which makes it a problem to use the function in a portable application.

To be honest, if I need to quote someone to make my argument stronger, it would be Chris: https://ramblings.implicit.net/c/2014/05/02/c-functions-that-should-be-avoided-part-2.html

Agree with the points above -- by itself (without additional context) "use strlcpy instead of strcpy" is bad advice, as the naive approach just replaces a strcpy that does not check for overflow beforehand with a strlcpy that does not check the return value for overflow. Indeed if you look at strlcpy callers in the tree you see that checking the return value is rare.