From: Oskari Pirhonen <xxc3ncoredxx@gmail.com>
To: Alejandro Colomar <alx@kernel.org>
Cc: Matthew House <mattlloydhouse@gmail.com>,
Jonny Grant <jg@jguk.org>, linux-man <linux-man@vger.kernel.org>
Subject: Re: strncpy clarify result may not be null terminated
Date: Fri, 10 Nov 2023 01:06:44 -0600 [thread overview]
Message-ID: <ZU3WhDRpJj0GWnSp@dj3ntoo> (raw)
In-Reply-To: <ZUzPNydLkFPEvvsa@debian>
[-- Attachment #1: Type: text/plain, Size: 5095 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2023-11-10 17:50 UTC|newest]
Thread overview: 138+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-04 11:27 strncpy clarify result may not be null terminated Jonny Grant
2023-11-04 19:33 ` Alejandro Colomar
2023-11-04 21:18 ` Jonny Grant
2023-11-05 1:36 ` Alejandro Colomar
2023-11-05 21:16 ` Jonny Grant
2023-11-05 23:31 ` Alejandro Colomar
2023-11-07 11:52 ` Jonny Grant
2023-11-07 13:23 ` Alejandro Colomar
2023-11-07 14:19 ` Jonny Grant
2023-11-07 16:17 ` Alejandro Colomar
2023-11-07 17:00 ` Jonny Grant
2023-11-07 17:20 ` Alejandro Colomar
2023-11-08 6:18 ` Oskari Pirhonen
2023-11-08 9:51 ` Alejandro Colomar
2023-11-08 9:59 ` Thorsten Kukuk
2023-11-08 15:09 ` Alejandro Colomar
[not found] ` <6bcad2492ab843019aa63895beaea2ce@DB6PR04MB3255.eurprd04.prod.outlook.com>
2023-11-08 15:44 ` Thorsten Kukuk
2023-11-08 17:26 ` Adhemerval Zanella Netto
2023-11-08 14:06 ` Zack Weinberg
2023-11-08 15:07 ` Alejandro Colomar
2023-11-08 19:45 ` G. Branden Robinson
2023-11-08 21:35 ` Carlos O'Donell
2023-11-08 22:11 ` Alejandro Colomar
2023-11-08 23:31 ` Paul Eggert
2023-11-09 0:29 ` Alejandro Colomar
2023-11-09 10:13 ` Jonny Grant
2023-11-09 11:08 ` catenate vs concatenate (was: strncpy clarify result may not be null terminated) Alejandro Colomar
2023-11-09 14:06 ` catenate vs concatenate Jonny Grant
2023-11-27 14:33 ` catenate vs concatenate (was: strncpy clarify result may not be null terminated) Zack Weinberg
2023-11-27 15:08 ` Alejandro Colomar
2023-11-27 15:13 ` Alejandro Colomar
2023-11-27 16:59 ` G. Branden Robinson
2023-11-27 18:35 ` Zack Weinberg
2023-11-27 23:45 ` G. Branden Robinson
2023-11-09 11:13 ` strncpy clarify result may not be null terminated Alejandro Colomar
2023-11-09 14:05 ` Jonny Grant
2023-11-09 15:04 ` Alejandro Colomar
2023-11-08 19:04 ` DJ Delorie
2023-11-08 19:40 ` Alejandro Colomar
2023-11-08 19:58 ` DJ Delorie
2023-11-08 20:13 ` Alejandro Colomar
2023-11-08 21:07 ` DJ Delorie
2023-11-08 21:50 ` Alejandro Colomar
2023-11-08 22:17 ` [PATCH] stpncpy.3, string_copying.7: Clarify that st[rp]ncpy() do NOT produce a string Alejandro Colomar
2023-11-08 23:06 ` Paul Eggert
2023-11-08 23:28 ` DJ Delorie
2023-11-09 0:24 ` Alejandro Colomar
2023-11-09 14:11 ` Jonny Grant
2023-11-09 14:35 ` Alejandro Colomar
2023-11-09 14:47 ` Jonny Grant
2023-11-09 15:02 ` Alejandro Colomar
2023-11-09 17:30 ` DJ Delorie
2023-11-09 17:54 ` Andreas Schwab
2023-11-09 18:00 ` Alejandro Colomar
2023-11-09 19:42 ` Jonny Grant
2023-11-09 7:23 ` Oskari Pirhonen
2023-11-09 15:20 ` [PATCH v2 1/2] " Alejandro Colomar
2023-11-09 15:20 ` [PATCH v2 2/2] stpncpy.3, string.3, string_copying.7: Clarify that st[rp]ncpy() pad with null bytes Alejandro Colomar
2023-11-10 5:47 ` Oskari Pirhonen
2023-11-10 10:47 ` Alejandro Colomar
2023-11-08 2:12 ` strncpy clarify result may not be null terminated Matthew House
2023-11-08 19:33 ` Alejandro Colomar
2023-11-08 19:40 ` Alejandro Colomar
2023-11-09 3:13 ` Matthew House
2023-11-09 10:26 ` Jonny Grant
2023-11-09 10:31 ` Jonny Grant
2023-11-09 11:38 ` Alejandro Colomar
2023-11-09 12:43 ` Alejandro Colomar
2023-11-09 12:51 ` Xi Ruoyao
2023-11-09 14:01 ` Alejandro Colomar
2023-11-09 18:11 ` Paul Eggert
2023-11-09 23:48 ` Alejandro Colomar
2023-11-10 5:36 ` Paul Eggert
2023-11-10 11:05 ` Alejandro Colomar
2023-11-10 11:47 ` Alejandro Colomar
2023-11-10 17:58 ` Paul Eggert
2023-11-10 18:36 ` Alejandro Colomar
2023-11-10 20:19 ` Alejandro Colomar
2023-11-10 23:44 ` Jonny Grant
2023-11-10 19:52 ` Alejandro Colomar
2023-11-10 22:14 ` Paul Eggert
2023-11-11 21:13 ` Alejandro Colomar
2023-11-11 22:20 ` Paul Eggert
2023-11-12 9:52 ` Jonny Grant
2023-11-12 10:59 ` Alejandro Colomar
2023-11-12 20:49 ` Paul Eggert
2023-11-12 21:00 ` Alejandro Colomar
2023-11-12 21:45 ` Alejandro Colomar
2023-11-13 23:46 ` Jonny Grant
2023-11-17 21:57 ` Jonny Grant
2023-11-18 10:12 ` Alejandro Colomar
2023-11-18 23:03 ` Jonny Grant
2023-11-10 11:36 ` Jonny Grant
2023-11-10 13:15 ` Alejandro Colomar
2023-11-18 23:40 ` Jonny Grant
2023-11-20 11:56 ` Jonny Grant
2023-11-20 15:12 ` Alejandro Colomar
2023-11-20 23:08 ` Jonny Grant
2023-11-20 23:42 ` Alejandro Colomar
2023-11-10 11:23 ` Jonny Grant
2023-11-09 12:23 ` Alejandro Colomar
2023-11-09 12:35 ` Alejandro Colomar
2023-11-10 7:06 ` Oskari Pirhonen [this message]
2023-11-10 11:18 ` Alejandro Colomar
2023-11-11 7:55 ` Oskari Pirhonen
2023-11-10 16:06 ` Matthew House
2023-11-10 17:48 ` Alejandro Colomar
2023-11-13 15:01 ` Matthew House
2023-11-11 20:55 ` Jonny Grant
2023-11-11 21:15 ` Jonny Grant
2023-11-11 22:36 ` Alejandro Colomar
2023-11-11 23:19 ` Alejandro Colomar
2023-11-17 21:46 ` Jonny Grant
2023-11-18 9:37 ` PDF book of unreleased pages (was: strncpy clarify result may not be null terminated) Alejandro Colomar
2023-11-19 0:22 ` Deri
2023-11-19 1:19 ` Alejandro Colomar
2023-11-19 9:29 ` Alejandro Colomar
2023-11-19 16:21 ` Deri
2023-11-19 20:58 ` Alejandro Colomar
2023-11-20 0:46 ` G. Branden Robinson
2023-11-20 9:43 ` Alejandro Colomar
2023-11-18 9:44 ` NULL safety " Alejandro Colomar
2023-11-18 23:21 ` NULL safety Jonny Grant
2023-11-24 22:25 ` Alejandro Colomar
2023-11-25 0:57 ` Jonny Grant
2023-11-10 10:40 ` strncpy clarify result may not be null terminated Stefan Puiu
2023-11-10 11:06 ` Jonny Grant
2023-11-10 11:20 ` Alejandro Colomar
2023-11-12 9:17 ` [PATCH 0/2] Expand BUGS section of string_copying(7) Alejandro Colomar
2023-11-12 9:18 ` [PATCH 1/2] string_copying.7: BUGS: *cat(3) functions aren't always bad Alejandro Colomar
2023-11-12 9:18 ` [PATCH 2/2] string_copying.7: BUGS: Document strl{cpy,cat}(3)'s performance problems Alejandro Colomar
2023-11-12 11:26 ` [PATCH v2 0/3] Improve string_copying(7) Alejandro Colomar
2023-11-12 11:26 ` [PATCH v2 1/3] string_copying.7: BUGS: *cat(3) functions aren't always bad Alejandro Colomar
2023-11-17 21:43 ` Jonny Grant
2023-11-18 0:25 ` Signing all patches and email to this list Matthew House
2023-11-18 23:24 ` Jonny Grant
2023-11-12 11:26 ` [PATCH v2 2/3] string_copying.7: BUGS: Document strl{cpy,cat}(3)'s performance problems Alejandro Colomar
2023-11-12 11:27 ` [PATCH v2 3/3] strtcpy.3, string_copying.7: Add strtcpy(3) Alejandro Colomar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZU3WhDRpJj0GWnSp@dj3ntoo \
--to=xxc3ncoredxx@gmail.com \
--cc=alx@kernel.org \
--cc=jg@jguk.org \
--cc=linux-man@vger.kernel.org \
--cc=mattlloydhouse@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).