Re: [PATCH] avoid buffer overflow in sunrpc clnt_create (BZ #22542)

["Dmitry V. Levin" <ldv at altlinux dot org>]

On Tue, Feb 06, 2018 at 04:54:45PM -0800, Paul Eggert wrote:
> On 02/06/2018 01:06 PM, Dmitry V. Levin wrote:
> > On Mon, Dec 04, 2017 at 12:04:12AM -0800, Paul Eggert wrote:  >> Dmitry V. Levin wrote: >>> Do people really expect that? Assuming 
> that people are aware >>> of linux kernel behaviour, why would they 
> expect that? >> >> These days, it's because strncpy format is obsolete 
> and is not something >> programmers are ordinarily aware of. When in 
> doubt (which there seems to be >> here), glibc should use 
> null-terminated strings rather than strncpy format. > > Is there any 
> statistics what programmers are ordinarily aware of? > > I have no 
> doubts that some valid code[1] no longer compiles with > 
> -Werror=stringop-truncation, and the only plausible fix is to mark > 
> struct sockaddr_un.sun_path with __attribute_nonstring__. If memory 
> serves, a problem is that some glibc-related uses of struct 
> sockaddr_un.sun_path assume strncpy format and others do not. If so, 
> marking it with __attribute_nonstring__ will pacify uses in some cases 
> (such as the one you mention), while masking bugs in others. If there's 
> some doubt about the intended format of this buffer, then it's safer for 
> everybody to stick to null-terminated strings in it.
> 
> For unusual programs such as the test program you mention, it's easy 
> enough to use memcpy instead of strncpy to test buffers that are not 
> null-terminated. Using memcpy would be better for this kind of test 
> anyway, since it would allow a test case where the null byte at the end 
> of the string is not followed by nulls all the way to the end of the buffer.

I don't see why a valid code has to be maimed to cater for broken code.
If anything, it's broken code that has to be changed, not otherwise.

I hope you don't really suggest people to apply changes like this
to their valid code:
-	strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));
+	memcpy(addr.sun_path, av[1], strlen(av[1]) < sizeof(addr.sun_path) ? strlen(av[1]) + 1 : sizeof(addr.sun_path));


-- 
ldv

Attachment: signature.asc
Description: PGP signature