From owner-freebsd-current@FreeBSD.ORG Mon Nov 19 22:28:55 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 45B05C14; Mon, 19 Nov 2012 22:28:55 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wi0-f172.google.com (mail-wi0-f172.google.com [209.85.212.172]) by mx1.freebsd.org (Postfix) with ESMTP id 999B48FC08; Mon, 19 Nov 2012 22:28:54 +0000 (UTC) Received: by mail-wi0-f172.google.com with SMTP id hm9so349871wib.13 for ; Mon, 19 Nov 2012 14:28:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=dlmaEM37Ofjw5U1p2b2TnN2fRMsHFAC8HRRqolxEQK8=; b=xZTtTlNdrq1a2AqaPaIq2WxOJ1D1FKbBjngsiUwjO+g42wTT/YgYY5Jpn7h6BVN5JG iSszQcbHZP0ct9y3e8X+ymwJEnU39QEzbQ1O2y3DTe4+Mz8M8jro/0rpknwkESYP3jlh D/ps1eH3uqTSEbDQBpFnitF++FlZEgX8ZcBpyOKzz+jFEDH9hvqqiccCfceIf/8Ad7uI EnKxSzxOThvhK9W+SLD6sza37N4t72y6mO2BIdoRjZ7CyqcZFCBAKoUMOcn5h3VULwaY sLL7VcDaMfRLql+z5YgJUQTiSDX1zsXwjMtdJRB3W1z7tQGNpzOtTq3ZUvBVchWnC4ig fPSQ== Received: by 10.180.86.70 with SMTP id n6mr11277620wiz.4.1353364133464; Mon, 19 Nov 2012 14:28:53 -0800 (PST) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPS id i2sm15348380wiw.3.2012.11.19.14.28.52 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 19 Nov 2012 14:28:52 -0800 (PST) Date: Mon, 19 Nov 2012 23:28:43 +0100 From: Mateusz Guzik To: Ryan Stone Subject: Re: pw keeps setting /etc/group to 0600 Message-ID: <20121119222843.GB22292@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Ryan Stone , FreeBSD Current , bapt@freebsd.org References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) Cc: bapt@freebsd.org, FreeBSD Current X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.14 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, 19 Nov 2012 22:28:55 -0000 On Sat, Nov 17, 2012 at 11:20:21AM -0500, Ryan Stone wrote: > My original complaint that /etc/group gets permissions of 0600 is a result > of a bug in libutil, which bapt@ ported pw to use in r242349. The new > group manipulation API using mktemp to create a temporary file, writes the > new group database to the temp file and then renames the temp file to > /etc/group. The problem here is that mktemp creates a file with a mode of > 600, and libutil never chmods it. That should be pretty trivial to fix. My additional 0,03$: I took closer look to this and I think that problems are much broader than this. I don't know if similar problems were present before. First, pw should not fail if other instance is running, it should wait instead (think of parallel batch scripts adding some users/groups). Second, current code has a race: lockfd = open(group_file, O_RDONLY, 0); if (lockfd < 0 || fcntl(lockfd, F_SETFD, 1) == -1) err(1, "%s", group_file); if (flock(lockfd, LOCK_EX|LOCK_NB) == -1) { [..] gr_copy(pfd, tfd, gr, old_gr); /* copy from groupfile to tempfile */ [..] rename(tempfile,groupfile); Now let's consider threads A and B: A: open() A: lock(); A: gr_copy B: open() Now B has file descriptor to /etc/group that is about to be removed. A: rename() A: unlock() B: lock() Now B has a lock on unlinked file. B: gr_copy() B: rename() ... and stores new content losing modifications done by A Third, I don't like current api. gr_lock and gr_tmp have no arguments (that matter anyway) gr_copy operates on two descriptors given as arguments gr_mkdb takes nothing and is expected to do The Right Thing I think descriptos should be hidden. Also I think that passing around struct gr_something (sorry, never had talent for names) that would contain all necessary data would be nice. -- Mateusz Guzik