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 > <[email protected]> or <[email protected]> > 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; [email protected]; svn-src-hea > [email protected] > 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 > > <[email protected]> or <[email protected]> > > 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; [email protected]; svn-src- > > hea > > [email protected] > > Subject: Re: svn commit: r336619 - head/lib/libc/gen > > > > On Mon, 2018-07-23 at 13:11 -0700, Cy Schubert wrote: > > > > > > > > > In message <[email protected]>, 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 > > > > > <[email protected]> or <[email protected]> > > > > > 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 > > _______________________________________________ [email protected] mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "[email protected]"
