From owner-svn-src-head@freebsd.org Tue Jul 24 02:25:19 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C4FFB1034345; Tue, 24 Jul 2018 02:25:19 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from smtp-out-no.shaw.ca (smtp-out-no.shaw.ca [64.59.134.13]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id F27F379BFC; Tue, 24 Jul 2018 02:25:18 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from spqr.komquats.com ([70.67.125.17]) by shaw.ca with ESMTPA id hn0efUWI2p5A1hn0gfeQ5A; Mon, 23 Jul 2018 20:25:11 -0600 X-Authority-Analysis: v=2.3 cv=JLKPTPCb c=1 sm=1 tr=0 a=VFtTW3WuZNDh6VkGe7fA3g==:117 a=VFtTW3WuZNDh6VkGe7fA3g==:17 a=8nJEP1OIZ-IA:10 a=R9QF1RCXAYgA:10 a=6I5d2MoRAAAA:8 a=YxBL1-UpAAAA:8 a=XCIwSH-Hwn2F5QzIN-oA:9 a=6FbIkE3uWnDEfb4f:21 a=KIJR5KywLR3eh5VY:21 a=wPNLvfGTeEIA:10 a=UJ0tAi3fqDAA:10 a=IjZwj45LgO3ly-622nXo:22 a=Ia-lj3WSrqcvXOmTRaiG:22 Received: from slippy.cwsent.com (slippy [10.1.1.91]) by spqr.komquats.com (Postfix) with ESMTPS id 8E75D191C; Mon, 23 Jul 2018 19:25:15 -0700 (PDT) Received: from slippy.cwsent.com (localhost [127.0.0.1]) by slippy.cwsent.com (8.15.2/8.15.2) with ESMTP id w6O2PA1K004914; Mon, 23 Jul 2018 19:25:10 -0700 (PDT) (envelope-from Cy.Schubert@cschubert.com) Received: from slippy (cy@localhost) by slippy.cwsent.com (8.15.2/8.15.2/Submit) with ESMTP id w6O2PAZ3004911; Mon, 23 Jul 2018 19:25:10 -0700 (PDT) (envelope-from Cy.Schubert@cschubert.com) Message-Id: <201807240225.w6O2PAZ3004911@slippy.cwsent.com> X-Authentication-Warning: slippy.cwsent.com: cy owned process doing -bs X-Mailer: exmh version 2.8.0 04/21/2012 with nmh-1.7.1 Reply-to: Cy Schubert From: Cy Schubert X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.cschubert.com/ To: Ian Lepore cc: Cy Schubert , Alan Somers , src-committers , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r336619 - head/lib/libc/gen In-Reply-To: Message from Ian Lepore of "Mon, 23 Jul 2018 20:18:40 -0600." <1532398720.1344.206.camel@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Date: Mon, 23 Jul 2018 19:25:10 -0700 X-CMAE-Envelope: MS4wfLKeF6jobuwlkpN1kZX78DFdSSKIIHfwA2TaEvLwEo52jTZylCs6gVrVnr9KqttpGXReNOhFM1+/MwePlvAdjjpBjnVhW5Wi0rbgpUT9YnCFKl/xw/JH dYrIZTbn1VgpYZX86dxCZxSOp63cG3bypbSObcTZFC3uNQnmvtT37S9xUP/xYAVm8p+G2kmAaqLMTirFqiCZ7/e1DxE3X2NcqSly6V4LcNWJDjzITPWCVZLb GwCermThDB+cChEtYIL4YnHhyzS2XPs79FFKp65feP/Ierbgvbob+c6e6SxALEl0XNQ0oUtQMK6Qk4RFuXRH3e7KPDXAUcqKel5CVWswyBY= X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Jul 2018 02:25:20 -0000 In message <1532398720.1344.206.camel@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 FreeBSD UNIX: 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 > > or > > 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@freebsd.org; svn-src-hea > > d@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 > > > or > > > 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@freebsd.org; svn-src- > > > hea > > > d@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@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 > > > > > > or > > > > > > 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 > > >