From owner-svn-src-all@freebsd.org Wed Nov 18 23:20:41 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 D11A2474A58; Wed, 18 Nov 2020 23:20:41 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4CbzLx5bfFz4nks; Wed, 18 Nov 2020 23:20:41 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id 0AD993C0199; Wed, 18 Nov 2020 23:20:35 +0000 (UTC) Date: Wed, 18 Nov 2020 23:20:34 +0000 From: Brooks Davis To: Stefan Esser Cc: Jessica Clarke , src-committers , svn-src-all , svn-src-head Subject: Re: svn commit: r367813 - head/lib/libutil Message-ID: <20201118232034.GE1158@spindle.one-eyed-alien.net> 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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="mvpLiMfbWzRoNl4x" Content-Disposition: inline In-Reply-To: <9f719272-f1e4-5668-02d2-bd2fbd0e5d6a@freebsd.org> User-Agent: Mutt/1.9.4 (2018-02-28) X-Rspamd-Queue-Id: 4CbzLx5bfFz4nks X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] 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, 18 Nov 2020 23:20:41 -0000 --mvpLiMfbWzRoNl4x Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 19, 2020 at 12:05:51AM +0100, Stefan Esser wrote: > Am 18.11.20 um 23:14 schrieb Jessica Clarke: > > On 18 Nov 2020, at 21:52, Stefan Esser wrote: > >> Am 18.11.20 um 22:15 schrieb Jessica Clarke: > >>> On 18 Nov 2020, at 19:44, Stefan E??er wrote: > >>>> + /* > >>>> + * Check for some other thread already having > >>>> + * set localbase - this should use atomic ops. > >>>> + * The amount of memory allocated above may leak, > >>>> + * if a parallel update in another thread is not > >>>> + * detected and the non-NULL pointer is overwritten. > >>>> + */ > >>> Why was this committed with a known racy/leaky implementation? > >> > >> Because the alternatives that I offered for discussion were > >> less acceptable. > >=20 > > That has no bearing over whether this one is. >=20 > Three alternatives have been discussed: >=20 > 1) static buffer of size MAXPATHLEN > 2) dynamically allocated buffer (as committed) > 3) dynamically allocated buffer filled by a constructor >=20 > 1) has been rejected, since it adds 1 KB of bss to each program > that is linked with libutil, whether it uses getlocalbase() or > not. >=20 > 3) has been rejected since it causes 1 getenv() and 2 sysctl() > calls when the program linked with libutil starts, independently > of whether getlocalbase() is used or not. >=20 > 2) has been selected, since it is only called when needed and it > does not pre-allocate a large buffer. >=20 > Which other alternative do you want to suggest? >=20 > Did you have a look at the review announced on the -current list > and mentioned in the commit? >=20 > >>> What happens if I set the value with a sysctl and call this? > >> > >> You'll get the value set with sysctl, unless overridden by the > >> environment variable. There is a window of a few nano-seconds > >> where a thread executing in parallel on another core might be > >> able to set the localbase variable (between the test for NULL > >> in this function and the assignment to it). The value that will > >> be returned by either thread will be identical, so there is no > >> risk of corruption of the result. > >=20 > > But if I call getlocalbase, then set it via sysctl, then call > > getlocalbase again, I see the old value. If, however, I omit the first > > getlocalbase, I see the new value. This differs from how getenv/setenv > > of the value work, where you always see the up-to-date value. Maybe you > > think that's a feature, but it's something to watch out for and > > explicitly call out in the documentation. >=20 > Actual programs call getlocalbase() exactly once and create the > required pathes from the value returned. >=20 > It is possible to copy the value from the environment to a buffer, > but at added complexity and introducing another race condition. >=20 > The program itself might have modified its environment and then > use an inconsistent value when it calls getlocalbase() again > thereafter. But why would you want to do this - it seems to be > a complicated way lof foot-shooting to me. >=20 > I'm not a native speaker of English and not best qualified to > clearly express this in the man-page. Feel free to commit any > clarification or suggest an addition for me to commit. >=20 > > You also misunderstand all the subtleties of multithreading here. There > > are no acquire/release pairs so it is entirely legal for Thread 2 to > > read Thread 1's initialised value for localbase before the contents of > > it are visible (i.e. the pointer is initialised but the data is > > garbage). >=20 > Yes, and thread 2's value will be identical to the one from > thread 1, just in a different heap location, unless there is a > write to the sysctl variable in the kernel at just the same > time. And you cannot protect against this race and you'll get > either the old or new value. >=20 > > The `(volatile const char*)localbase` cast is also a complete waste of > > time. You probably meant to write `(const char * volatile)localbase` > > but even then that does nothing useful as the cast is too late. What > > you really were trying to write was > > `*(const char * volatile *)&localbase`, but you need proper atomics > > anyway for this to be safe. >=20 > I was not sure about the correct volatile declaration, I've got > to admit. It was in the review and I did not get any comments > regarding that specific modifier. The goal was to enforce the > access to the localbase pointer in memory after returning from > the sysctl to shorten the window. >=20 > Perhaps I should just have completely ignore any multi-threading > issues and accepted that an overlapping execution of this function > would allocate multiple sysctl destination buffers but loose all > references but one in the assignment to localbase. >=20 > It will not happen in practice, it just does not make sense to > call this function more than once in a program. >=20 > >> This unlikely case may actually leak a heap allocated string > >> of typically tens of bytes (but with negligible probability). > >> > >> But this really is a non-issue, since there should never be a > >> reason to invoke this function in a multi-threaded context. > >=20 > > 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. >=20 > 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. 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. > And unless the program modifies its own environment at run-time, > all invocsations of getlocalbase() are guaranteed to return the > same result again and again. >=20 > > Also if malloc returns NULL I would quite like that to be an error for > > the function and not silently fall back on _PATH_LOCALBASE. >=20 > The function is specifically specified to always succeed. Else > there is extra code required at each invocation to check for > errors. Not being able to allocate the required buffer of at > most 1 KB will cause every non-trivial program to fail anyway. >=20 > Returning the compiled in default value will direct the program > to access files in /usr/local, which is an administrator controlled > path, even on systems with non-default localbase. >=20 > Returning NULL is possible in that case, but the only ways a > program could deal with this case is to either crash, sue the > default path, or ignore the accesses to files in localbase. >=20 > In either case it will not operate as intended, but I consider > directing accesses to the FreeBSD default location (or the > modified _PATH_LOCALBASE) to be a reasonable fall-back. >=20 > But this will not happen unless provoked (by not allowing the > program to allocate any memory from the heap, which will cause > it to fail as soon as any other library function or start-up > code tries to allocate heap-space). >=20 > I had put this code up for review and it has been discussed > before (and another implementation has been reverted after > several iterations of changes). >=20 > If there had been consensus to return NULL, I could have > committed that - it is a trivial change and ENOMEM would have > been set after a malloc failure. >=20 > I have written about this possibility and I had appreciated > if comments had been made on Phabricator before the commit. 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 aim 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.) -- Brooks --mvpLiMfbWzRoNl4x Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJftaxCAAoJEKzQXbSebgfANd8H/3qF115Nw0JRvd7zwOpp9nAP njbdKJ+Pkc0NBYv2BV+up8cPVkKpp0vAtA+Hn7Aekbkqw2sOUW8ZDkHn+CiUEk95 gOVlaSfPLL9bSheVcYnw8iQeRR3vV+VTlpr+HTYwJjnncETpF/YF/8aES0izisK0 PJW7cxSkS29QPuTQf/gH5x7OcOcaAGfr3b4vWGWF4EsDvsWT2htzfZuIyKYzEGJf G3trYlY5xRDeVY3sC6COS1lzjMYM5wHX0VTAo12bFFDSBtfbQSCVh6tIFUHkx7hD xezSX+HiwWyMdrz0fEa1llIjeB7qhidlqMrDDaZKvFg7N9BVgFMkaHE3OuhqSVk= =SXLv -----END PGP SIGNATURE----- --mvpLiMfbWzRoNl4x--