From owner-freebsd-current@FreeBSD.ORG Mon Jan 9 19:59:44 2012 Return-Path: Delivered-To: current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B9D371065672 for ; Mon, 9 Jan 2012 19:59:44 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from gw.catspoiler.org (gw.catspoiler.org [75.1.14.242]) by mx1.freebsd.org (Postfix) with ESMTP id 9DD7A8FC1B for ; Mon, 9 Jan 2012 19:59:44 +0000 (UTC) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.13.3/8.13.3) with ESMTP id q09JwKZK029815; Mon, 9 Jan 2012 11:58:24 -0800 (PST) (envelope-from truckman@FreeBSD.org) Message-Id: <201201091958.q09JwKZK029815@gw.catspoiler.org> Date: Mon, 9 Jan 2012 11:58:20 -0800 (PST) From: Don Lewis To: des@des.no In-Reply-To: <86ty44ykj4.fsf@ds4.des.no> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Cc: current@FreeBSD.org Subject: Re: couldn't log on to my -CURRENT machine after upgrade to latest PAM X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jan 2012 19:59:44 -0000 On 9 Jan, Dag-Erling Smørgrav wrote: > Don Lewis writes: >> Dag-Erling Smørgrav writes: >> > The culprit was this commit: >> > >> > http://trac.des.no/openpam/changeset/487/trunk/lib/openpam_configure.c >> > >> > However, I'm not confident that simply reverting this commit is the >> > right way to go. >> Thanks for the detective work. It looks to me like the bug is caused by >> the change in the openpam_parse_chain() return value. In the previous >> code it returned the value of count, which I would guess was greater >> than zero if it found something. In that case, the for loop in >> openpam_load_chain() would be terminated because r != 0. In the new >> code, openpam_parse_chain() will return PAM_SUCCESS if it found >> something, and the loop in openpam_load_chain() will go through another >> iteration because ret == PAM_SUCCESS. > > Thank you, Captain Obvious. I am still not confident that simply > reverting this commit is the right way to go, because it discards > valuable information when an error occurs, especially if an error occurs > while parsing an include. It wasn't so obvious to me, especially with the gratuitous variable renaming in the diff. After staring at the code a lot more, I see your point about the loss of information. The problem is that openpam_parse_chain() returns PAM_SUCCESS whether or not if found anything, but we want the loop to terminate when either an error is detected or if openpam_parse_chain() actually found something. Maybe changing the loop exit to something like this would work: if (ret != PAM_SUCCESS || pamh->chains[facility] != NULL) return (ret);