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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 34724 Build 31781: arc lint + arc unit
Event Timeline
Comment Actions
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?
| lib/libutil/getlocalbase.3 | ||
|---|---|---|
| 55 | This line seems like a copy/paste error. | |
| 71 | s/call/caller/ | |
| 78 | 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. | |
| 81 | s/getpathname/getlocalbase/ ? | |
| lib/libutil/getlocalbase.c | ||
| 55 | 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 | ||
|---|---|---|
| 78 | strlcat() doesn't guarantee NULL termination of the result. Blame OpenBSD ;-) | |
| sbin/nvmecontrol/nvmecontrol.c | ||
|---|---|---|
| 188 | strlcpy(locallib + len, ... ) is better here.. | |
| usr.sbin/mailwrapper/mailwrapper.c | ||
| 112 | same as above. | |
| usr.sbin/pkg/pkg.c | ||
| 1055 | same as above. | |
| lib/libutil/getlocalbase.3 | ||
|---|---|---|
| 78 | strncat doesn't guarantee NUL termination either, which is why I recommended the strlcpy idiom | |
Comment Actions
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 | ||
|---|---|---|
| 65–66 | 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. | |
| lib/libutil/getlocalbase.3 | ||
|---|---|---|
| 52 | We usually use Va here instead of Em (as documented in the style.mdoc(5)) | |
| 55 | That should probably be Dv instead. | |
| 83 | getlocalbase is unnecessary here. | |
| 91 | +1 | |
| 98 | This is fine unless we want to MFC of course. | |
| 100 | +1, this should trigger a warning when you lint the manual with mandoc -Tlint and/or igor. | |
| 103 | Missing a period at the end. | |
Comment Actions
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 | What are we protecting from here? | |
Comment Actions
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.