Re: Possible unsafe use of strncat in sbin/sysctl/sysctl.c

[Marc Balmer <mbalmer%NetBSD.org@localhost>]

Am 23.08.11 23:23, schrieb Jimmy Johansson:
> Hi,
> 
> 
> I propose the following patch, as I think this is what the developer
> intended anyway:
> 
> Index: sbin/sysctl/sysctl.c
> ===================================================================
> RCS file: /cvsroot/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.136
> diff -u -w -p -r1.136 sysctl.c
> --- sbin/sysctl/sysctl.c        3 Aug 2011 01:58:30 -0000       1.136
> +++ sbin/sysctl/sysctl.c        23 Aug 2011 18:57:52 -0000
> @@ -579,8 +579,8 @@ print_tree(int *name, u_int namelen, str
>                 for (ni = 0; ni < namelen; ni++) {
>                         (void)snprintf(n, sizeof(n), "%d", name[ni]);
>                         if (ni > 0)
> -                               strncat(gdname, ".", sizeof(gdname));
> -                       strncat(gdname, n, sizeof(gdname));
> +                               (void)strlcat(gdname, ".", sizeof(gdname));
> +                       (void)strlcat(gdname, n, sizeof(gdname));
>                 }
>         }
>  
> @@ -589,11 +589,11 @@ print_tree(int *name, u_int namelen, str
>         else if (add) {
>                 snprintf(n, sizeof(n), "%d", pnode->sysctl_num);
>                 if (namelen > 1) {
> -                       strncat(gsname, sep, sizeof(gsname));
> -                       strncat(gdname, ".", sizeof(gdname));
> +                       (void)strlcat(gsname, sep, sizeof(gsname));
> +                       (void)strlcat(gdname, ".", sizeof(gdname));
>                 }
> -               strncat(gsname, pnode->sysctl_name, sizeof(gsname));
> -               strncat(gdname, n, sizeof(gdname));
> +               (void)strlcat(gsname, pnode->sysctl_name, sizeof(gsname));
> +               (void)strlcat(gdname, n, sizeof(gdname));
>         }
>  
>         if (Mflag && pnode != &my_root) {
> 
> I haven't looked at the code in detail so there might be code that
> checks that the second argument will always fit. I still believe that
> the way strncat is used here is unsafe and if it was used in a safe
> manner then I wouldn't have had to read the code in detail to know if it
> was safe.
> 
> If I read strncat(3) correctly safe use is either
> 
>   strncat(foo, bar, sizeof(foo) - strlen(foo) - 1);
> 
> or to use
> 
>   strlcat(foo, bar, sizeof(foo));
> 
> but not
> 
>   strncat(foo, bar, sizeof(foo));
> 
> as this will be wrong even if foo is empty because it doesn't allow for
> the terminating '\0'.
> 
> I think the developer intended to use strlcat, as the rest of the code
> uses strlcat and not strncat.

I don't like how functions, which return values are ignored, are casted
to (void).

Who invented that idiom?  What is that good for, other than making your
eyes bleed?

I think it is no more than quieting cc or lint.