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

[Martin Sebor <msebor at gmail dot com>]

On 02/07/2018 02:31 PM, Dmitry V. Levin wrote:
> On Wed, Feb 07, 2018 at 08:57:46AM -0700, Martin Sebor wrote:
> > On 02/06/2018 02: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__.
> > >
> > > I think we should revisit the patch submitted by Martin.
> > >
> > > [1] strace HEAD's tests no longer build in Fedora Rawhide with the following
> > > diagnostics:
> > > net-accept-connect.c: In function ‘main’:
> > > net-accept-connect.c:57:2: error: ‘strncpy’ specified bound 108 equals destination size [-Werror=stringop-truncation]
> > >   strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));
> >
> > I was going to follow up on the original thread but got stuck
> > trying to come up with a test case showing the kernel creating
> > a path with no terminating nul, and I've been too busy with
> > GCC work to get back to it.
> >
> > I'm also worried that annotating the member nonstring will
> > lead to many more false positives for the canonical use case
> > of computing the path length/size using strlen even on input
> > (to the kernel/library) than true positives for the elusive
> > cases when there is no nul on output.  (Attribute nonstring
> > causes warnings when an array or pointer declared with is
> > passed to strlen or other functions that expect a nul-
> > terminated string.)
>
> struct sockaddr_un.sun_path is not a nul-terminated string,
> one has to use strnlen instead of strlen.

It may not be guaranteed to be nul-terminated, but it is nul
terminated most of the time (when the path is shorter than
the array), otherwise there wouldn't be so much working code
that depends on it and so much documentation with examples
that assume it is, including Glibc's own manual and the Linux
man page:
  http://man7.org/linux/man-pages/man7/unix.7.html

Once sun_path is declared nonstring all code that follows
these examples and in which GCC cannot prove the string is
nul-terminated will trigger the same warning.  The only
difference will be that the warning will point to sun_path
being passed to strlen or as the second argument to strcpy.

The question on my mind is: will the true positives/bugs
the warning finds outnumber the false positives by enough
of a margin that the warning will be viewed as useful.
To be able to even start to answer the question I'd need
to see an example where the sun_path isn't nul-terminated
and get a sense of how frequently that might happen in
practice.

Martin