From owner-svn-src-all@FreeBSD.ORG Wed Jan 28 10:23:42 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 9AE3743E; Wed, 28 Jan 2015 10:23:42 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id CAEC5CE3; Wed, 28 Jan 2015 10:23:41 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id E4D2D1A2540; Wed, 28 Jan 2015 20:55:58 +1100 (AEDT) Date: Wed, 28 Jan 2015 20:55:57 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Robert Watson Subject: Re: svn commit: r277652 - in head/usr.sbin/pw: . tests In-Reply-To: Message-ID: <20150128195647.E1974@besplex.bde.org> References: <201501241913.t0OJD4xT039188@svn.freebsd.org> <20150125155254.V1007@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=Qdxf4Krv c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=mkU-gdjLa1_SUtGgM7IA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, Baptiste Daroussin , src-committers@freebsd.org, svn-src-all@freebsd.org, Bruce Evans X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Jan 2015 10:23:42 -0000 On Tue, 27 Jan 2015, Robert Watson wrote: > On Sun, 25 Jan 2015, Bruce Evans wrote: > >> Negative ids have historical abuses in places like mountd. mountd still >> hard-codes -2 and -2 for the default uid and gid of an unprivileged user. >> ... > > I'm sure it goes without saying, but for those that don't know (i.e., some > subset of people who are not Bruce): > > (-1) has a defined value both for our system-call interface (chown(2), > fchown(2), etc, use (-1) to indicate that no change is requested). > > This is also used inside the kernel to similar end, where VNOVAL also takes > on a value of (-1). > > This problem also used to exist in NFS, where in NFSv2, (-1) was also used to > indicate which fields not to update, but this was fixed in NFSv3 by > introducing discriminated unions. > > I personally find myself a fan of fixing (eliminating) VNOVAL, but in the end > it would likely just be disruptive and confusing. VNOVAL is kernel-only, so it need not limit values. But then you would have to escape the value that it usurps. ffs uses VNOVAL for (quoting an old version): % if (vap->va_rdev != VNOVAL) { % if ((vap->va_type != VNON) || (vap->va_nlink != VNOVAL) || % (vap->va_fsid != VNOVAL) || (vap->va_fileid != VNOVAL) || % (vap->va_blocksize != VNOVAL) || (vap->va_rdev != VNOVAL) || % ((int)vap->va_bytes != VNOVAL) || (vap->va_gen != VNOVAL)) { Some of these are magic or kernel-only or otherwise have plenty of out of band values. va_nlink is most interesting. It is only 16 bits signed. If it were not signed, then it couldn't represent VNOVAL. {LINK_MAX} is hard-coded as LINK_MAX = 32767. The hard-coded type and value are both broken for file systems that have larger or smaller limits. Removing the definition of LINK_MAX would fix this for limits up to 32767. The limit in ext2fs is 32000 and the limit in ext2fs is 65000. 65000 is not supported in FreeBSD. It doesn't fit in nlink_t. It leaves 536 out of band vales in a 16-bit unsigned nlink_t. One of these can be VNOVAL after type puns to make VNOVAL 65535. % if (vap->va_flags != VNOVAL) { Flags have enough spare bits. va_flags is u_long, so type puns occur implicitly in this comparison. The flags type(s) have even more design botches than older types. A mixture of signed int, uint32_t and u_long is used for flags variables. % * Go through the fields and update iff not VNOVAL. % if (vap->va_uid != (uid_t)VNOVAL || vap->va_gid != (gid_t)VNOVAL) { Here the type puns are done explicitly (so they are not really puns). The types here are unsigned, so there are no unportabilities in these conversions. It is just accidental that VNOVAL = -1 matches the POSIX magic number -1 (even before the conversions). The code depends on this. % if (vap->va_size != VNOVAL) { A size of -1 is impossible, but va_size is unsigned so the usual type puns occur and the out of band value is actually 0xffffffffffffffff. That should be large enough for anyone. It is currently physically impossible, and also theoretically impossible, since file offsets are signed so they can only reach half as high. % if (vap->va_atime.tv_sec != VNOVAL || % vap->va_mtime.tv_sec != VNOVAL || % vap->va_birthtime.tv_sec != VNOVAL) { % if (vap->va_atime.tv_sec != VNOVAL) % if (vap->va_mtime.tv_sec != VNOVAL) % if (vap->va_birthtime.tv_sec != VNOVAL && % if (vap->va_atime.tv_sec != VNOVAL) { % if (vap->va_mtime.tv_sec != VNOVAL) { % if (vap->va_birthtime.tv_sec != VNOVAL && time_t is signed, so the out of band value is 1 second before the Epoch, not (via type puns) 1 second before the time_t's roll over. This magic value is also reserved for reporting errors in C (for mktime(), etc.). Error handling is especially difficult for APIs like mktime(), strtol(), and some syscalls since the error value is also a supported in-band. For strtol(), it is a very normal value. In C, the representation can be anything so the error value can be for any time. Implementations should avoid using it for a useful time, but portable applications cannot depend on this. In POSIX, the error value must be 1 second before the Epoch and that is an interesting time. % if (vap->va_mode != (mode_t)VNOVAL) { The 16-bit mode_t is not quite full (type 0170000 is free), so VNOVAL is out of band. Oops. There would be no problem even if mode_t were full, since it gets promoted and since it is unsigned it cannot promote to -1. Similarly for nlink_t if it were unsigned. Bruce