Re: [PATCH 3/4] strncat.3 fixes

[Alejandro Colomar <alx@kernel.org>]

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

Hi Paul,

On Sun, Nov 12, 2023 at 03:52:07PM -0800, Paul Eggert wrote:

> Don't say "concatenate".

Ok

> Use "byte" instead of "character",

Ok

> and use standalone terminology rather than relying on the
> reader already having read string_copying(7).

I need to check again in a standalone commit.

> Don't say "width" when "size" was intended.

Ok

> Fix indenting of prototype.

Ok

> Simplify possible implementation, fixing a bug when the
> source string length and sz exceed INT_MAX.

Heh!  Good.

> Say that strncat is rarely useful.

Do we need to say that, or is it already implied by
"append non-null bytes from a source array to a string,
 and null-terminate the result"?
Not many programs need to do that operation.  I'm fine with saying it's
rarely useful; I'm just wondering if it's worth it.

> Say that behavior is undefined if the destination is not a string.

Ok

> Simplify example by using plain sizeof rather than an nitems macro,

If you want sizeof(), please use sizeof(), not sizeof.

I use nitems() with these functions because if you switch to wide
strings, you can keep the nitems() part, while you'd have to change it
if you had sizeof().  Also, nitems() makes it safe against sizeof(ptr).
What do you think of this?

> by removing a confusingly-named 'maxsize' local,

Ok

> and by removing an unnecessary call to 'exit'.

This was practice from Michael Kerrisk, which I like: always terminate
the program with exit(1); don't rely on just ending the scope of main().
That way, it's more visual.

Please split all these things into separate patches, if you don't mind,
and sign the patch.

> ---
>  man3/strncat.3 | 54 +++++++++++++++++++++-----------------------------
>  1 file changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/man3/strncat.3 b/man3/strncat.3
> index d0f777d36..9a7df474a 100644
> --- a/man3/strncat.3
> +++ b/man3/strncat.3
> @@ -5,7 +5,8 @@
>  .\"
>  .TH strncat 3 (date) "Linux man-pages (unreleased)"
>  .SH NAME
> -strncat \- concatenate a null-padded character sequence into a string
> +strncat \- append non-null bytes from a source array to a string,
> +and null-terminate the result
>  .SH LIBRARY
>  Standard C library
>  .RI ( libc ", " \-lc )
> @@ -14,15 +15,18 @@ Standard C library
>  .B #include <string.h>
>  .P
>  .BI "char *strncat(char *restrict " dst ", const char " src "[restrict ." sz ],
> -.BI "               size_t " sz );
> +.BI "              size_t " sz );
>  .fi
>  .SH DESCRIPTION
> -This function catenates the input character sequence
> -contained in a null-padded fixed-width buffer,
> -into a string at the buffer pointed to by
> +This function appends at most
> +.I sz
> +non-null bytes from the array pointed to by
> +.I src
> +to the end of the string pointed to by
>  .IR dst .
> -The programmer is responsible for allocating a destination buffer large enough,
> -that is,
> +.I dst
> +must point to a string contained in a buffer that is large enough,
> +that is, the buffer size must be at least
>  .IR "strlen(dst) + strnlen(src, sz) + 1" .
>  .P
>  An implementation of this function might be:
> @@ -32,12 +36,7 @@ An implementation of this function might be:
>  char *
>  strncat(char *restrict dst, const char *restrict src, size_t sz)
>  {
> -    int   len;

Oops!  :)

> -    char  *p;
> -\&
> -    len = strnlen(src, sz);
> -    p = dst + strlen(dst);
> -    p = mempcpy(p, src, len);
> +    char *p = mempcpy(dst + strlen(dst), src, strnlen(src, sz));

Please use a C89 declaration for p (top of the function).

>      *p = \[aq]\e0\[aq];
>  \&
>      return dst;
> @@ -67,11 +66,12 @@ C11, POSIX.1-2008.
>  .SH HISTORY
>  POSIX.1-2001, C89, SVr4, 4.3BSD.
>  .SH CAVEATS
> -The name of this function is confusing.
> -This function has no relation to
> +The name of this function is confusing, as it has no relation to
>  .BR strncpy (3).

I didn't connect both sentences because I think it is confusing at
several levels.  Not only it has no relation to strncpy(), but it is
neither to writing 'n' bytes.  But yeah, having no relation to strncpy()
is the main one, so we could simplify.  What do you think?

Thanks!
Alex

> +This function is rarely useful in practice.
>  .P
> -If the destination buffer is not large enough,
> +If the destination buffer does not already contain a string,
> +or is not large enough,
>  the behavior is undefined.
>  See
>  .B _FORTIFY_SOURCE
> @@ -91,40 +91,32 @@ Shlemiel the painter
>  #include <stdlib.h>
>  #include <string.h>
>  \&
> -#define nitems(arr)  (sizeof((arr)) / sizeof((arr)[0]))
> -\&
>  int
>  main(void)
>  {
> -    size_t  maxsize;
> -\&
> -    // Null-padded fixed-width character sequences
> +    // Null-padded fixed-size character sequences
>      char    pre[4] = "pre.";
>      char    new_post[50] = ".foo.bar";
>  \&
>      // Strings
>      char    post[] = ".post";
>      char    src[] = "some_long_body.post";
> -    char    *dest;
> -\&
> -    maxsize = nitems(pre) + strlen(src) \- strlen(post) +
> -              nitems(new_post) + 1;
> -    dest = malloc(sizeof(*dest) * maxsize);
> +    char *dest = malloc(sizeof pre + strlen(src) \- strlen(post)
> +                        + sizeof new_post + 1);
>      if (dest == NULL)
>          err(EXIT_FAILURE, "malloc()");
>  \&
> -    dest[0] = \[aq]\e0\[aq];  // There's no 'cpy' function to this 'cat'.
> -    strncat(dest, pre, nitems(pre));
> +    dest[0] = \[aq]\e0\[aq];  // There's no `cpy' function to this `cat'.
> +    strncat(dest, pre, sizeof pre);
>      strncat(dest, src, strlen(src) \- strlen(post));
> -    strncat(dest, new_post, nitems(new_post));
> +    strncat(dest, new_post, sizeof new_post);
>  \&
>      puts(dest);  // "pre.some_long_body.foo.bar"
>      free(dest);
> -    exit(EXIT_SUCCESS);
>  }
>  .EE
>  .\" SRC END
>  .in
>  .SH SEE ALSO
>  .BR string (3),
> -.BR string_copying (3)
> +.BR string_copying (7)
> -- 
> 2.41.0
> 
> 

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

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