Re: strncpy clarify result may not be null terminated

Alejandro Colomar
2023-11-10T17:50:34Z

[-- Attachment #1: Type: text/plain, Size: 2970 bytes --]

Hi Oskari,

On Fri, Nov 10, 2023 at 01:06:44AM -0600, Oskari Pirhonen wrote:
> On Thu, Nov 09, 2023 at 13:23:14 +0100, Alejandro Colomar wrote:
> > 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 ;)

Hmmm, it just caught me after a year fixing broken strncpy(3) calls. I
was a bit unfair. I'm sorry if I wasn't so nice. Hopefully, we've all
learnt something about string-copying functions. :)

> > 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.

I'll write it then! :D

>
> ... 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);

A NULL check shouldn't be necessary (no other copying functions have,
and that's not a big deal with them, although I have mixed feelings
about things like memcpy(dst, NULL, 0)).

About strnlen(3), you're right, and Paul also pointed that out. See the
other mail I sent to the list with an inline implementation of strxcpy()
using strnlen(3).

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

I always assume this. If not, please ask your vendor to provide a POSIX
layer. Or at least the parts of POSIX that can be implemented in a
free-standing implementation. Or stop using that vendor.

>
> 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...

Looks good at first glance. :)

Cheers,
Alex

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

--
<https://www.alejandro-colomar.es/>