svn commit: r367813 - head/lib/libutil

[Stefan Esser]

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.
> 
> 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.
> 
> 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.)

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20201125/2c2f5259/attachment.sig>