Re: [PATCH 5/6] sunrpc: Suppress GCC -Os warning on user2netname

[Carlos O'Donell <carlos@redhat.com>]

On Wed, Sep 21, 2022 at 10:51:07AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> GCC with -Os warns that sprint might overflow:
> 
>   netname.c: In function ‘user2netname’:
>   netname.c:51:28: error: ‘%s’ directive writing up to 255 bytes into a
>   region of size between 239 and 249 [-Werror=format-overflow=]
>      51 |   sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
>         |                            ^~               ~~~~~~~
>   netname.c:51:3: note: ‘sprintf’ output between 8 and 273 bytes into a
>   destination of size 256
>      51 |   sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
>         |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   cc1: all warnings being treated as errors
> 
> However the code does test prior the sprintf call that dfltdom plus
> the required extra space for OPSYS, uid, and extra character will not
> overflow and return 0 instead.

LGTM.

No regressions on x86_64.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  sunrpc/netname.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sunrpc/netname.c b/sunrpc/netname.c
> index bf7f0b81c4..c1d1c43e50 100644
> --- a/sunrpc/netname.c
> +++ b/sunrpc/netname.c
> @@ -20,6 +20,7 @@
>  #include <string.h>
>  #include <rpc/rpc.h>
>  #include <shlib-compat.h>
> +#include <libc-diag.h>

OK.

>  
>  #include "nsswitch.h"
>  
> @@ -48,7 +49,12 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid,
>    if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN)
>      return 0;
>  
> +  /* GCC with -Os warns that sprint might overflow while handling dfltdom,
> +     however the above test does check if an overflow would happen.  */
> +  DIAG_PUSH_NEEDS_COMMENT;
> +  DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wformat-overflow");
>    sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
> +  DIAG_POP_NEEDS_COMMENT;

OK. Agreed. The code above does that check.

>    i = strlen (netname);
>    if (netname[i - 1] == '.')
>      netname[i - 1] = '\0';
> -- 
> 2.34.1
>