[PATCH] sunrpc: use snprintf to guard against buffer overflow

[Martin Sebor]

On 12/3/20 3:09 AM, Florian Weimer via Libc-alpha wrote:
> * Philipp Tomsich:
> 
>> GCC11 has improved detection of buffer overflows detectable through the analysis
>> of format strings and parameters, which identifies the following issue:
>>     netname.c:52:28: error: '%s' directive writing up to 255 bytes into a region
>>                             of size between 239 and 249 [-Werror=format-overflow=]
>>
>> This rewrites user2netname() to use snprintf to guard against overflows.
>>
>> ---
>>
>>   sunrpc/netname.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/sunrpc/netname.c b/sunrpc/netname.c
>> index 24ee519..62e644f 100644
>> --- a/sunrpc/netname.c
>> +++ b/sunrpc/netname.c
>> @@ -49,8 +49,10 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid,
>>     if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN)
>>       return 0;
>>   
>> -  sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
>> -  i = strlen (netname);
>> +  i = snprintf (netname, MAXNETNAMELEN + 1, "%s.%d@%s", OPSYS, uid, dfltdom);
>> +  if (i > (size_t) MAXNETNAMELEN)
>> +    return 0;
>> +
> 
> There's a strlen check right above the sprintf, and it looks like it
> would catch cases where we'd right too much into the buffer.  So this
> looks like a GCC 11 false positive to me.  Am I missing something?

There has been a change to the warning but I don't see it in
my x86_64 build with the latest GCC/Glibc.  Compiling with
-ftree-dump-strlen=/dev/stdout prints the details for each
directive.  That should help explain how the warning came
up with the number.  My result looks like this:

netname.c:52: sprintf: objsize = 256, fmtstr = "%s.%d@%s"
   Directive 1 at offset 0: "%s"
     Result: 4, 4, 4, 4 (4, 4, 4, 4)
   Directive 2 at offset 2: ".", length = 1
     Result: 1, 1, 1, 1 (5, 5, 5, 5)
   Directive 3 at offset 3: "%d"
     Result: 1, 1, 11, 11 (6, 6, 16, 16)
   Directive 4 at offset 5: "@", length = 1
     Result: 1, 1, 1, 1 (7, 7, 17, 17)
   Directive 5 at offset 6: "%s"
     Result: 0, 237, 237, 237 (7, 244, 254, 254)
   Directive 6 at offset 8: "", length = 1

If you see the warning on a target other that x86_64 I'll be
happy to look into it if you can send me the translation unit
(and the target) so I can easily reproduce it on my end.

Martin

> 
> The switch to snprintf is reasonable (with the caveat that this code is
> in very, very deep maintenance mode), but I think you should replace the
> strlen check and also check for negative i.
> 
> Thanks,
> Florian
>