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

[Ian Lepore]

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.

-- 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
> >