[PATCH] strcat.3: SYNOPSIS: Fix the size of 'dest'

[PATCH] strcat.3: SYNOPSIS: Fix the size of 'dest'

[Alejandro Colomar]

I had a mistake when adding VLA syntax to this prototype.  From this
fixed prototype, it's visible how broken the design for this function
is.  Next move is to kill this function.

Cc: <libc-alpha@sourceware.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---

Hi!

I'm continuing my indiscriminated shooting against broken functions.
I don't remember if I ever used it, but it got me surprised for how much
broken it is.

Please kill this function in glibc.  The updated prototype using a bit
of imagination to overextend VLA syntax to show how it behaves, shows
how broken it is.

It is impossible to use this function correctly (okay, it you try hard,
you can, but only for the pleasure of using it without crashing, not for
anything useful).

Cheers,

Alex

 man3/strcat.3 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/man3/strcat.3 b/man3/strcat.3
index a4a376ba9..61d3e54f1 100644
--- a/man3/strcat.3
+++ b/man3/strcat.3
@@ -20,9 +20,8 @@ .SH SYNOPSIS
 .B #include <string.h>
 .PP
 .BI "char *strcat(char *restrict " dest ", const char *restrict " src );
-.BI "char *strncat(char " dest "[restrict ." n "], \
-const char " src "[restrict ." n ],
-.BI "              size_t " n );
+.BI "char *strncat(char " dest "[restrict strlen(." dest ") + strnlen(." n ") + 1],"
+.BI "              const char " src "[restrict ." n "], size_t " n );
 .fi
 .SH DESCRIPTION
 The
-- 
2.38.1

Re: [PATCH] strcat.3: SYNOPSIS: Fix the size of 'dest'

[Russ Allbery]

Alejandro Colomar via Libc-alpha <libc-alpha@sourceware.org> writes:

> I'm continuing my indiscriminated shooting against broken functions.
> I don't remember if I ever used it, but it got me surprised for how much
> broken it is.

> Please kill this function in glibc.  The updated prototype using a bit
> of imagination to overextend VLA syntax to show how it behaves, shows
> how broken it is.

> It is impossible to use this function correctly (okay, it you try hard,
> you can, but only for the pleasure of using it without crashing, not for
> anything useful).

Hi Alejandro,

I'm just a bystander here, but I've seen a few proposals like this go by,
and I'm not sure anyone has yet said explicitly that these functions are
incredibly widely used in real C code out in the world.  They predate all
of the functions that you are recommending as replacements for them, so
they're common in old, portable C.

I think the work you're doing to document that they should never be used
for new code in the man pages is great; please continue!  Although it may
be useful to carry with that some caveats about portability; some of the
replacements you've mentioned are going to pose portability challenges.

I'm a bit more dubious about glibc marking these functions as formally
deprecated unless the C standard also deprecates them (as happened with
gets, which is probably the best precedent for what you're trying to
accomplish).

For the record, there is quite a lot of code out there that uses strcpy
and strcat safely, using a pattern where first the total length of the
resulting string is calculated, that much space is allocated, and then it
is assembled with strcpy and strcat.  This used to be the standard way to
assemble strings in C before strlcpy or asprintf existed (and neither of
those functions are all that portable even now; I would still hesittate to
use either without a configure probe and a replacement implementation).  I
think you're exaggerating the difficulty of using them safely a tiny bit,
but maybe that's just because I'm an old C programmer for whom that
pattern is a finger macro.

It's also probably worth saying explicitly that strlcpy and strlcat are
still quite dangerous functions and need to be used very carefully.  They
do solve the problem of buffer overflows when used properly, but they
replace that problem with silent truncation, which can also cause security
vulnerabilities in the right circumstances.

-- 
Russ Allbery (eagle@eyrie.org)             <https://www.eyrie.org/~eagle/>

Re: [PATCH] strcat.3: SYNOPSIS: Fix the size of 'dest'

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 5203 bytes --]

Hi Russ!

On 12/5/22 18:48, Russ Allbery wrote:
> Alejandro Colomar via Libc-alpha <libc-alpha@sourceware.org> writes:
> 
>> I'm continuing my indiscriminated shooting against broken functions.
>> I don't remember if I ever used it, but it got me surprised for how much
>> broken it is.
> 
>> Please kill this function in glibc.  The updated prototype using a bit
>> of imagination to overextend VLA syntax to show how it behaves, shows
>> how broken it is.
> 
>> It is impossible to use this function correctly (okay, it you try hard,
>> you can, but only for the pleasure of using it without crashing, not for
>> anything useful).
> 
> Hi Alejandro,
> 
> I'm just a bystander here, but I've seen a few proposals like this go by,
> and I'm not sure anyone has yet said explicitly that these functions are
> incredibly widely used in real C code out in the world.

Yeah, I'd say sadly.

>  They predate all
> of the functions that you are recommending as replacements for them, so
> they're common in old, portable C.

Well, the building blocks for implementing any of the less-portable functions 
are already portable: memcpy(3), memccpy(3), strlen(3), ...

> 
> I think the work you're doing to document that they should never be used
> for new code in the man pages is great; please continue!

Thanks!  Will do :)

>  Although it may
> be useful to carry with that some caveats about portability; some of the
> replacements you've mentioned are going to pose portability challenges.

libbsd already does a good job of making them portable:

<https://libbsd.freedesktop.org/wiki/>

It's available in virtually all Linux distributions, and then there are ports to 
other systems such as Solaris.  I guess if some system is not yet supported by 
it, one could port it.

> 
> I'm a bit more dubious about glibc marking these functions as formally
> deprecated unless the C standard also deprecates them (as happened with
> gets, which is probably the best precedent for what you're trying to
> accomplish).

Someone has to propose its cold death.  Otherwise, we may live with it for 
decades, if not centuries, and people will continue to comit crimes in their 
names.  :)

> 
> For the record, there is quite a lot of code out there that uses strcpy
> and strcat safely,

Oh, I didn't (yet?) kill those.  I don't recommend them personally, but gcc 
fortifications have improved so much that it's harder to commit crimes with 
them.  There are other ways to get the same simplicity as with those, and even 
better performance, but they are certainly still useful in some cases.

The ones I killed so far are strNcpy(3) and strNcat(3), which simply can't be 
used safely.

> using a pattern where first the total length of the
> resulting string is calculated, that much space is allocated, and then it
> is assembled with strcpy and strcat.  This used to be the standard way to
> assemble strings in C before strlcpy or asprintf existed (and neither of
> those functions are all that portable even now; I would still hesittate to
> use either without a configure probe and a replacement implementation).

If you use autotools, you can follow what we did in shadow for getting 
readpassphrase(3bsd) from libbsd:

<https://github.com/shadow-maint/shadow/pull/573>

>  I
> think you're exaggerating the difficulty of using them safely a tiny bit,
> but maybe that's just because I'm an old C programmer for whom that
> pattern is a finger macro.

No, I might be exaggerating a little bit if I said that about 
strcpy(3)/strcat(3).  But for strNcpy(3)/strNcat(3), I stand by my point: they 
are never the function you should use.


While you can use strncpy(3) or strncat(3) without breaking your program, they 
are so wrongly designed that it's really hard to not break it.  There's _always_ 
a simpler _and_ safer function for the job.  Also, their names don't document 
the intention of the programmer.

I prefer actual macros (or functions) rather than finger macros.  Finger macros, 
especially multi-line patterns, tend to hide bugs.  If there's any value in 
strncpy(3) (there isn't), hide it in a real macro and use the macro.  But if you 
do that, you can just use memcpy(3) for the implementation as well.

> 
> It's also probably worth saying explicitly that strlcpy and strlcat are
> still quite dangerous functions and need to be used very carefully.
>  They
> do solve the problem of buffer overflows when used properly, but they
> replace that problem with silent truncation, which can also cause security
> vulnerabilities in the right circumstances. >


Yes, truncation is a problem of strlcpy(3) (or actually, of any safe string copy 
functions).  But good string copy functions will at least make it easy for you 
to check for it.  strNcpy(3) and strNcat(3) actually don't even allow you to 
check for it, which is yet another reason to avoid them.

At least with strlcpy(3) you can find if you're checking for truncation by 
grepping it.  If you find '^ *strlcpy(', it's likely that you forgot to check.

Cheers,

Alex


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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]