From owner-svn-src-all@freebsd.org Wed Nov 25 11:48:51 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 16F5447F4D8; Wed, 25 Nov 2020 11:48:51 +0000 (UTC) (envelope-from se@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CgzgQ71WJz3C73; Wed, 25 Nov 2020 11:48:50 +0000 (UTC) (envelope-from se@freebsd.org) Received: from Stefans-MBP-WLAN.fritz.box (p200300cd5f0ff700ac60d7652b2f1231.dip0.t-ipconnect.de [IPv6:2003:cd:5f0f:f700:ac60:d765:2b2f:1231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: se/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id 340D96281; Wed, 25 Nov 2020 11:48:50 +0000 (UTC) (envelope-from se@freebsd.org) To: Brooks Davis Cc: Jessica Clarke , src-committers , svn-src-all , svn-src-head References: <202011181944.0AIJiUU3003699@repo.freebsd.org> <25465269-5497-4981-A1E4-CC1FFAB68CF4@freebsd.org> <92a87ed8-4b22-4d03-9481-b02f92dcaaa0@freebsd.org> <9f719272-f1e4-5668-02d2-bd2fbd0e5d6a@freebsd.org> <20201118232034.GE1158@spindle.one-eyed-alien.net> From: Stefan Esser Subject: Re: svn commit: r367813 - head/lib/libutil Message-ID: <8e050429-b0e9-d4dd-c723-f2ba2dd3d00f@freebsd.org> Date: Wed, 25 Nov 2020 12:48:48 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: <20201118232034.GE1158@spindle.one-eyed-alien.net> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Z5NFINb7crvPmXNGjZlvjqE1u2yCdWQdH" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.34 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, 25 Nov 2020 11:48:51 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Z5NFINb7crvPmXNGjZlvjqE1u2yCdWQdH Content-Type: multipart/mixed; boundary="WXpJi1DbZdEgugAkkDt68BRCoIIDlws0g"; protected-headers="v1" From: Stefan Esser To: Brooks Davis Cc: Jessica Clarke , src-committers , svn-src-all , svn-src-head Message-ID: <8e050429-b0e9-d4dd-c723-f2ba2dd3d00f@freebsd.org> Subject: Re: svn commit: r367813 - head/lib/libutil References: <202011181944.0AIJiUU3003699@repo.freebsd.org> <25465269-5497-4981-A1E4-CC1FFAB68CF4@freebsd.org> <92a87ed8-4b22-4d03-9481-b02f92dcaaa0@freebsd.org> <9f719272-f1e4-5668-02d2-bd2fbd0e5d6a@freebsd.org> <20201118232034.GE1158@spindle.one-eyed-alien.net> In-Reply-To: <20201118232034.GE1158@spindle.one-eyed-alien.net> --WXpJi1DbZdEgugAkkDt68BRCoIIDlws0g Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Am 19.11.20 um 00:20 schrieb Brooks Davis: > On Thu, Nov 19, 2020 at 12:05:51AM +0100, Stefan Esser wrote: >> Am 18.11.20 um 23:14 schrieb Jessica Clarke: >>> Why not? There could easily be code out there calling getenv in a >>> multi-threaded context so this is inadequate as a replacement. Yes it= 's >>> inefficient but it's perfectly legal and imaginable. >> >> Yes, calling getenv() might occur, but getlocalbase() is generally >> called before configuration files are accessed, and the resulting >> path is saved in the program. Other uses are possible, but this is >> the recurring pattern. >=20 > This seems like a very naive assumption. I could easily see libraries > wanting to know where localbase is and calling this completely without > knowledge of the application programmer. Yes, and what's the issue, then? The implementation that I provided could be called in a multi- threaded environment and from within libraries. It checked whether a new allocation was required and returned the pointer stored by another thread (but with identical contents) in case it lost a potential race that could exist once during the execution of this function for a fraction of a microsecond. Only if another thread had managed to store its pointer after the check for it being NULL simultaneously, then a few bytes could have been lost. You might have been able to trigger this with a specifically built test program. But I had only added the run-time allocation of that buffer (which might be exploited to leak at most a few KB or heap space) due to requests in the discussion. My initial version used a statically allocated buffer and was completely safe. >> I have written about this possibility and I had appreciated >> if comments had been made on Phabricator before the commit. >=20 > Don't mistake posting something for review with obtaining > consensus. I glanced at the review, but it contained no use cases or > justification for the feature so it was impossible to comment on the > implementation in the time I had. I still don't understand what you ai= m > to support and why (except that your implementation fails to support > things like per-jail or per-ABI localbase which both seem like things > people might want.) As described in the review, adding per-jail variables is a way to extend the usefulness of this function, but out-of-scope at this time. It requires changes to other parts of FreeBSD (kernel, jails) that might then lead to an update of this function, but which can be developed independently of the initial use of this function in the limit way currently supported by the kernel. And I do not need the run-time configurability at all. In fact, I'd replace getlocalbase() calls by a macro substitution that just returns _PATH_LOCALBASE as the default value the system was built with. But there has been interest in this feature and getenv("LOCALBASE") has been used in a number of programs to be able to manipulate that prefix at run-time. This implementation just simplifies the getenv() calls in that case, and that alone might be a justification for this function. It has the added security feature of checking issetugid() and not using the environment value in that case. Thus it simplifies programs and allows them to take advantage of other methods to configure LOCALBASE (e.g. later per jail or per ABI) without changes to the programs reaching into LOCALBASE. The getlocalbase() function had been suggested by Scott Long to provide a generic means to retrieve the LOCALBASE prefix in programs. Therefore, he implemented the getenv() functionality found in a number of programs, before. I had patched the calendar program to support data files provided by a port (deskutils/calendar-data) to ease maintenance of these date files outside of the base system. While doing this, I noticed the use of literal "/usr/local" in a number of base system utilities and provided this value as _PATH_LOCALBASE to make it easier to override, if desired (not needed by me, but again requested on the maillists in several threads). A non-default _PATH_LOCALBASE can be compiled into programs, but not in e.g. shell scripts. I have added the user.localbase sysctl to query _PATH_LOCALBASE from programs that do not have it compiled in (e.g. shell scripts). The user.localbase sysctl variable is queried and passed to the rc subsystem as ${_localbase} by changes committed by me in SVN rev. 367294. All these changes are meant to allow building a system that has a non-default LOCALBASE, without hunting down all literal occurances of "/usr/local" in the tree. Making sysctl("user.localbase") available in getlocalbase() is then a logical consequence of all the other changes. As I wrote before: I'm interested in providing a standard method to obtain LOCALBASE in case it is not set to the FreeBSD default value of "/usr/local" to allow as many components of the system to automatically use the correct paths in such an environment. The function should always return a string value that can be used as a prefix for a file name (i.e. should never return NULL or any other value that needs to be explicitly checked for by the caller). Added functionality, e.g. jail or ABI specific values should be supported by extended versions of this functions without a need to change or even re-compile the calling programs. I have created a new review for a proposed replacement of the current implementation as: https://reviews.freebsd.org/D27370 I highly prefer that implementation and it is based on my initial review version of D27236, which did not use a heap-allocated buffer. Regards, STefan --WXpJi1DbZdEgugAkkDt68BRCoIIDlws0g-- --Z5NFINb7crvPmXNGjZlvjqE1u2yCdWQdH Content-Type: application/pgp-signature; name="OpenPGP_signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="OpenPGP_signature" -----BEGIN PGP SIGNATURE----- wsB5BAABCAAjFiEEo3HqZZwL7MgrcVMTR+u171r99UQFAl++RKAFAwAAAAAACgkQR+u171r99UQr lgf/YNYtULm/KAKtnSZ+Ko6XqGrXV7R+/ajyNzBChqxgIPEq4kbZaChgT1lPXq+98z+K9z0EDmo8 7e7Zt1Nu9EPguAdpyrpfg5tdHxtrm5NaBUidAwArRV00t+2V33dH973ZHJMh6BVjMgd1TBqyInHv GG+8hckebvgH5nECoaKp1YACK3vC0/R8HkV0MYU10I6n2P5VDMNDjb4SGU+xfKwamejyE/s/mcrI 7xNZgxSpqZX9z9oP16mZ1wppxyW761oEeH35CuqECNPPu++cM/+MzgMaJmn2R25R1YOSiOh/jw7C XKnMtNE9ZF9K7wBToPB/MbYnl2WefzxKh25qHN6zfA== =jfza -----END PGP SIGNATURE----- --Z5NFINb7crvPmXNGjZlvjqE1u2yCdWQdH--