On Thu, Nov 09, 2023 at 13:23:14 +0100, Alejandro Colomar wrote:

... snip ...

> > > > For the sake of reference, I looked into a few big C and C++ projects to
> > > > see how often a strncpy(3)-based snippet was used to produce a truncated
> > > > copy. I found 18 instances in glibc 2.38, 2 in util-linux 2.39.2 (in spite
> > > > of its custom xstrncpy() function), 61 in GNU binutils 2.41, 43 in
> > > > GDB 13.2, 1 in LLVM 17.0.4, 7 in CPython 3.12.0, 99 in OpenJDK 22+22,
> > > > 10 in .NET Runtime 7.0.13, 3 in V8 12.1.82, and 86 in Firefox 120.0. (Note
> > > > that I haven't filtered out vendored dependencies, so there's a little bit
> > > > of double-counting.) It seems like most codebases that don't ban strncpy(3)
> > > > use a derived snippet somewhere or another. Also, I found 3 instances in
> > > > glibc 2.38 and 5 instances in Firefox 120.0 of detecting truncation by
> > > > checking the last character.
> > >
> > > I know.  I've been rewriting the code handling strings in shadow-utils
> > > for the last year, and ther was a lot of it.  I fixed several small bugs
> > > in the process, so I recommend avoiding it.
> > 
> > I can't tell you about your own experience, but in mine, the root cause of
> > most string-handling bugs has been excessive cleverness in using the
> > standard string functions, rather than the behavior of the functions
> > themselves. So one worry of mine is that if strncpy(3) ends up being
> > deprecated or whatever, then authors of portable libraries will start
> > writing lots of custom memcpy(3)-based replacements to their strncpy(3)-
> > based snippets, and more lines of code will introduce more opportunities
> > for cleverness.
> 
> Don't worry.  strncpy(3) won't be deprecated, thanks to tar(1).  ;)
> 

Just please don't tar and feather [1] the people who use it ;)

... snip ...

> > > > the code to understand the concept behind how these two snippets work, that
> > > > the only difference between the strncpy(3)'s special "character sequence"
> > > > and an ordinary C string is an additional null terminator at the end of the
> > > > destination buffer.
> > >
> > > This is part of string_copying(7):
> > >
> > > DESCRIPTION
> > >    Terms (and abbreviations)
> > >      string (str)
> > >             is  a  sequence  of zero or more non‐null characters followed by a
> > >             null byte.
> > >
> > >      character sequence
> > >             is a sequence of zero or  more  non‐null  characters.   A  program
> > >             should  never use a character sequence where a string is required.
> > >             However, with appropriate care, a string can be used in the  place
> > >             of a character sequence.
> > >
> > > I think that is very explicit in the difference.  strncpy(3) refers to
> > > that page for understanding the differences, so I think it is
> > > documented.
> > >
> > > strncpy(3):
> > > CAVEATS
> > >      The  name  of  these  functions  is confusing.  These functions produce a
> > >      null‐padded character sequence, not a string (see string_copying(7)).
> > 
> > My point is isn't that the difference is undocumented, but that the typical
> > man page reader isn't reading the man pages for their own sake, but because
> > they're looking at some code, and they want to Know What It's Doing as soon
> > as possible.
> 
> We could maybe add a list of ways people have tried to be clever with
> strncpy(3) in the past and failed, and then explain why those uses are
> broken.  This could be in a BUGS section.
> 

This would be a very fun read.

... snip ...

> > > Also, I've seen a lot of off-by-one bugs in calls to strncpy(3), so no,
> > > it's not correct code.  It's rather dangerous code that just happens to
> > > not be vulnerable most of the time.
> > 
> > So will all the custom strlen(3)+memcpy(3)-based replacements suddenly be
> > immune to off-by-one bugs?
> 
> Slightly.  Here's the typical use of strlen(3)+strcpy(3):
> 
> if (strlen(src) >= dsize)
> 	goto error;
> strcpy(dst, src);
> 
> There's no +1 or -1 in that code, so it's hard to make an off-by-one
> mistake.  Okay, you may have seen that it has a '>=', which one could
> accidentally replace by a '>', causing an off-by-one.  I'd wrap that
> thing in a strxcpy() wrapper so you avoid repetition. 
> 

Might I go so far as to recommend strnlen(3) instead of strlen(3)? That
way, instead of blindly looking for a null terminator, you stop after a
predetermined max length. Especially nice for untrusted input where you
can't make assumptions on the "fitness for a purpose" of what's being
fed in.

    if (src == NULL || strnlen(src, dsize) == dsize)
        goto error;
    strcpy(dst, src);

This, of course, assumes you have POSIX at your disposal.

I'm writing this before going to bed. I did briefly sanity check it with
a simple test prog, but it would be quite ironic if I missed something
wouldn't it...

- Oskari

[1]: https://en.wikipedia.org/wiki/Tarring_and_feathering