svn commit: r336619 - head/lib/libc/gen

[Cy Schubert]

In message <1532398720.1344.206.camel at freebsd.org>, Ian Lepore writes:
> I don't think ssh is necessarily broken. It assumes pw_class will not
> be NULL after a call to getpwnam(). That may be a fair assumption,
> there's nothing in the manpage about it, but in practice the struct has
> always been initialized to have pointers to empty strings. I changed
> that so that one of them started getting chagned to a NULL pointer
> without realizing it was breaking a sort of hidden implicit contract.
>
> I think the right fix is to make libutil's pw_scan() do the same thing
> getpwnam() and other callers of __pw_scan() do, and init the struct to
> have pointers to empty strings. And we should update some manpages to
> make that an explicit contract.

My previous email.


-- 
Cheers,
Cy Schubert <Cy.Schubert at cschubert.com>
FreeBSD UNIX:  <cy at FreeBSD.org>   Web:  http://www.FreeBSD.org

	The need of the many outweighs the greed of the few.


>
> -- Ian
>
> On Mon, 2018-07-23 at 18:47 -0700, Cy Schubert wrote:
> > Well, the ssh client is still broken and IMO needs to be fixed sooner
> > than later. If we want to maintain compatibility right now before any
> > rewrite is attempted, our options are limited. Just saying that if
> > this will be broken for a while we need some kind of workaround.
> > Instead of a null pointer a pointer to a statically defined null
> > string. I know it's ugly and hate to suggest it but it gives us (you)
> > more time for a better solution.
> > 
> > BTW, I agree about the whack-a-mole thing given the brokenness.
> > 
> > ---
> > Sent using a tiny phone keyboard.
> > Apologies for any typos and autocorrect.
> > Also, this old phone only supports top post. Apologies.
> > 
> > Cy Schubert
> > <Cy.Schubert at cschubert.com> or <cy at freebsd.org>
> > The need of the many outweighs the greed of the few.
> > ---
> > 
> > -----Original Message-----
> > From: Ian Lepore
> > Sent: 23/07/2018 17:05
> > To: Cy Schubert
> > Cc: Alan Somers; src-committers; svn-src-all at freebsd.org; svn-src-hea
> > d at freebsd.org
> > Subject: Re: svn commit: r336619 - head/lib/libc/gen
> > 
> > I don't wanna play whack-a-mole like that. This whole area is a great
> > big mess, and it needs a great big cleanup, and I've got too many
> > other
> > irons in the fire right now to do that.
> > 
> > -- Ian
> > 
> > On Mon, 2018-07-23 at 16:52 -0700, Cy Schubert wrote:
> > > 
> > > I think you misunderstood me because of my terse email. Sorry about
> > > that. We can address this with a NULL check in openssh's misc.c. I
> > > can't recall the actual path. I'll post a patch, which will explain
> > > it better than I can in English, as soon as I get home.
> > > 
> > > ---
> > > Sent using a tiny phone keyboard.
> > > Apologies for any typos and autocorrect.
> > > Also, this old phone only supports top post. Apologies.
> > > 
> > > Cy Schubert
> > > <Cy.Schubert at cschubert.com> or <cy at freebsd.org>
> > > The need of the many outweighs the greed of the few.
> > > ---
> > > 
> > > -----Original Message-----
> > > From: Ian Lepore
> > > Sent: 23/07/2018 13:40
> > > To: Cy Schubert
> > > Cc: Alan Somers; src-committers; svn-src-all at freebsd.org; svn-src-
> > > hea
> > > d at freebsd.org
> > > Subject: Re: svn commit: r336619 - head/lib/libc/gen
> > > 
> > > On Mon, 2018-07-23 at 13:11 -0700, Cy Schubert wrote:
> > > > 
> > > > 
> > > > In message <1532364679.1344.161.camel at freebsd.org>, Ian Lepore
> > > > writes:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Mon, 2018-07-23 at 09:41 -0700, Cy Schubert wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > I'm sure. Rolling this libc commit back addressed the ssh
> > > > > > segfaults
> > > > > > on all my systems.
> > > > > > 
> > > > > > ---
> > > > > > Sent using a tiny phone keyboard.
> > > > > > Apologies for any typos and autocorrect.
> > > > > > Also, this old phone only supports top post. Apologies.
> > > > > > 
> > > > > > Cy Schubert
> > > > > > <Cy.Schubert at cschubert.com> or <cy at freebsd.org>
> > > > > > The need of the many outweighs the greed of the few.
> > > > > > ---
> > > > > > 
> > > > > My current working theory is that some of the software that
> > > > > uses
> > > > > __pw_scan() pre-stages a pointer-to-empty-string into the
> > > > > pw_class
> > > > > field and my change ruined that by replacing it with a NULL
> > > > > pointer.
> > > > > Other callers of __pw_scan() don't do that, they just assume
> > > > > they're
> > > > > running as root and will get all the fields populated.
> > > > Yes. A simple check for pw->pw_class != NULL should fix this
> > > > instance.
> > > > 
> > > > 
> > > No, that doesn't work, because __pw_scan just skips setting
> > > pw_class
> > > and that leaves non-NULL garbage in that pointer.  There is a
> > > pw_fields
> > > field that uses bits to indicate which fields were parsed, but
> > > almost
> > > nothing uses it, so I'm reluctant to rely on it because this same
> > > kind
> > > of unexpected crash of some random tool will happen if there's
> > > anything
> > > that fills out a pwd struct without making those flags valid then
> > > calls
> > > pw_copy() which would rely on them being valid.
> > > 
> > > I've decided that the only safe way to fix this is to have
> > > pw_scan()
> > > do
> > > the same thing getpwnam() and friends do:  first init the pwd
> > > struct
> > > to
> > > known values, including supplying empty strings rather than NULL
> > > pointers for all the char* fields, before calling __pw_scan().
> > > 
> > > But, all of a sudden I've gotten busy with $work, so I may have to
> > > set
> > > all this aside for a few days.
> > > 
> > > -- Ian
> > >