Page MenuHomeFreeBSD

Introduce getlocalbase.3
ClosedPublic

Authored by scottl on Oct 30 2020, 4:51 PM.

Details

Reviewers
imp
se
bapt
bdrewery
0mp
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
manpages
Commits
rS367686: Add the library function getlocalbase and its manual page. This helps to
Summary

Implement getlocalbase.3 in libutil. This function looks at the LOCALBASE
environment, user.localbase sysctl, _PATH_LOCALBASE compile time variable,
and the fallback of /usr/local, in that order.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 30 2020, 4:51 PM

I like it generally. I wonder what to do about shell. I think the following

: ${LOCALBASE:-$(sysctl -n user.localbase)}

might do the trick and should be in the getlocalbase man page maybe?

rpokala added inline comments.
lib/libutil/getlocalbase.3
54 ↗(On Diff #78956)

This line seems like a copy/paste error.

70 ↗(On Diff #78956)

s/call/caller/

77 ↗(On Diff #78956)

Why do we need to return the resulting string length at all? Can't the caller just strlen() if it needs it? Requiring a pointer prevents using the obvious getlocalbasenam(buf, sizeof(buf)) idiom.

80 ↗(On Diff #78956)

s/getpathname/getlocalbase/

?

lib/libutil/getlocalbase.c
54 ↗(On Diff #78956)

You've already checked for path == NULL and returned ENOMEM, and haven't touched path since, so this will never be true.

lib/libutil/getlocalbase.3
77 ↗(On Diff #78956)

This is so strncat() can be used on the result. I'm fine with following your suggestion if others agree.

lib/libutil/getlocalbase.c
54 ↗(On Diff #78956)

I should have used tmppath here

lib/libutil/getlocalbase.3
77 ↗(On Diff #78956)

Do it my way, and use strlcat() instead of strncat(). :-)

80 ↗(On Diff #78956)

s/getlocalpath/getlocalbase/

?

lib/libutil/getlocalbase.c
58 ↗(On Diff #78961)

Again, path can't be NULL here.

scottl added inline comments.
lib/libutil/getlocalbase.3
77 ↗(On Diff #78956)

strlcat() doesn't guarantee NULL termination of the result. Blame OpenBSD ;-)

sbin/nvmecontrol/nvmecontrol.c
188 ↗(On Diff #78963)

strlcpy(locallib + len, ... ) is better here..
stlrcat will guarantee NUL termination here, but strncat will not when MAXPATHLEN - len == sizeof("/lib/nvmecontrol"), but strlcpy is still better since its interface is more clear cut.

usr.sbin/mailwrapper/mailwrapper.c
112 ↗(On Diff #78963)

same as above.

usr.sbin/pkg/pkg.c
1055 ↗(On Diff #78963)

same as above.

lib/libutil/getlocalbase.3
77 ↗(On Diff #78956)

strncat doesn't guarantee NUL termination either, which is why I recommended the strlcpy idiom

I'm still in doubt about the usefulness of LOCALBASE being specified at run-time if it is not applied to all paths in the system.
And for all other cases (not changed at run-time) a compiled in _PATH_LOCALBASE works as well, while scripts can use the sysctl command.

The sysctl variable user.localbase is available during the multi-user initialization and will be used to start some programs and scripts.
It could be set as a boot-time parameter (tunable), but any later change will lead to inconsistencies and may lead to unexpected behavior and security hazards (not only if the LOCALBASE environment variable was used in SUID programs, but possibly also in programs invoked by the user with system supplied configuration files in LOCALBASE).

I'd rather get the static configurable as provided by the read-only user.localbase sysctl variable supported in all relevant system components before getlocalbase() is maybe later extended to also consider the environment.
One possible use of LOCALBASE in the environment could be to support jails with a non-default localbase, but then only if it was provided as a read-only value. I'd rather make the sysctl variable private to the jail instead, though.

lib/libutil/getlocalbase.c
64–65 ↗(On Diff #78963)

I had expected that getlocalbase() fails if the passed in pathlen does not allow for the whole tmppath string to be passed back to the caller.

xtouqh_icloud.com added inline comments.
lib/libutil/getlocalbase.3
40 ↗(On Diff #78963)

.Fn getlocalbase "char *ssize_t"

50 ↗(On Diff #78963)

is checked

72 ↗(On Diff #78963)

New sentence, new line.

90 ↗(On Diff #78963)

This .El seems to be out of context.

97 ↗(On Diff #78963)

History section needs to say about function implementation history, not man page.

99 ↗(On Diff #78963)

Redundant before .Sh, remove.

Update recent improvements

This revision is now accepted and ready to land.Tue, Nov 10, 4:09 PM
scottl marked 2 inline comments as not done.Tue, Nov 10, 4:30 PM
0mp requested changes to this revision.Wed, Nov 11, 11:32 AM
0mp added a subscriber: 0mp.
0mp added inline comments.
lib/libutil/getlocalbase.3
90 ↗(On Diff #78963)

+1

99 ↗(On Diff #78963)

+1, this should trigger a warning when you lint the manual with mandoc -Tlint and/or igor.

52 ↗(On Diff #79388)

We usually use Va here instead of Em (as documented in the style.mdoc(5))

55 ↗(On Diff #79388)

That should probably be Dv instead.

83 ↗(On Diff #79388)

getlocalbase is unnecessary here.

98 ↗(On Diff #79388)

This is fine unless we want to MFC of course.

103 ↗(On Diff #79388)

Missing a period at the end.

This revision now requires changes to proceed.Wed, Nov 11, 11:32 AM

The problem I see is that not everything will use this or the sysctl. Ports, Poudriere, nearly everything, are doing getenv("LOCALBASE"). Other places have things like LOCALBASE?= /usr/local. So then I wonder what the usefulness of it being configured in the kernel in the first place is as a user setting it won't achieve much.

I might argue it makes more sense to make it call this getlocalbase(3) when getenv("LOCALBASE") is done but that's crazy right? LOCALBASE typically should be set once on a system down in something like /etc/login.conf and otherwise exported in build scripts.

I certainly understand the want to unify all of these LOCALBASE?= /usr/local and _PATH_LOCALBASE things but the interface really needs to be prioritizing the environment where its primary consumer is looking.

lib/libutil/getlocalbase.c
53 ↗(On Diff #79388)

What are we protecting from here?
I must not understand something. Wouldn't this cause any su or sudo to not respect the environment value?

The problem I see is that not everything will use this or the sysctl. Ports, Poudriere, nearly everything, are doing getenv("LOCALBASE"). Other places have things like LOCALBASE?= /usr/local. So then I wonder what the usefulness of it being configured in the kernel in the first place is as a user setting it won't achieve much.

I might argue it makes more sense to make it call this getlocalbase(3) when getenv("LOCALBASE") is done but that's crazy right? LOCALBASE typically should be set once on a system down in something like /etc/login.conf and otherwise exported in build scripts.

I certainly understand the want to unify all of these LOCALBASE?= /usr/local and _PATH_LOCALBASE things but the interface really needs to be prioritizing the environment where its primary consumer is looking.

Consider sudo and and create config files in /tmp/etc that allow everybody to gain a root shell - then call sudo with LOCALBASE=/tmp ...

If the alternate LOCALBASE is provided by a sysctl that can only be set at boot time (or by root at run-time, if we want to allow that) then commands like sudo will use that authorized value.
If you can pass any LOCALBASE of your choice to some SUID programs (like sudo) then you have just offered root access to all local users.

This revision was not accepted when it landed; it landed in state Needs Revision.Sat, Nov 14, 5:58 PM
This revision was automatically updated to reflect the committed changes.