Changeset View
Standalone View
lib/libutil/getlocalbase.c
Property | Old Value | New Value |
---|---|---|
svn:eol-style | null | native \ No newline at end of property |
svn:keywords | null | FreeBSD=%H \ No newline at end of property |
svn:mime-type | null | text/plain \ No newline at end of property |
/*- | |||||
* SPDX-License-Identifier: BSD-2-Clause | |||||
* | |||||
* Copyright 2020 Stefan Eßer <se@freebsd.org> | |||||
* | |||||
* Redistribution and use in source and binary forms, with or without | |||||
* modification, are permitted provided that the following conditions | |||||
* are met: | |||||
* 1. Redistributions of source code must retain the above copyright | |||||
* notice, this list of conditions and the following disclaimer. | |||||
* 2. Redistributions in binary form must reproduce the above copyright | |||||
* notice, this list of conditions and the following disclaimer in the | |||||
* documentation and/or other materials provided with the distribution. | |||||
* | |||||
* THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND | |||||
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |||||
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | |||||
* ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE | |||||
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |||||
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | |||||
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | |||||
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | |||||
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | |||||
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | |||||
* SUCH DAMAGE. | |||||
*/ | |||||
#include <sys/cdefs.h> | |||||
__FBSDID("$FreeBSD"); | |||||
#include <sys/param.h> | |||||
#include <sys/sysctl.h> | |||||
#include <sys/limits.h> | |||||
#include <stdlib.h> | |||||
#include <paths.h> | |||||
cem: style(9):
```
If <sys/cdefs.h> is needed
for __FBSDID(), include it first. If… | |||||
Done Inline ActionsYes, I had to add sys/param.h for MAXPATHLEN and forgot to remove the include of sys/types.h at that time. This will be fixed before the commit. se: Yes, I had to add sys/param.h for MAXPATHLEN and forgot to remove the include of sys/types.h at… | |||||
#include <libutil.h> | |||||
#include <unistd.h> | |||||
#ifndef _PATH_LOCALBASE | |||||
#define _PATH_LOCALBASE "/usr/local" | |||||
#endif | |||||
const char * | |||||
getlocalbase(void) | |||||
Done Inline ActionsDoesn't this static steal memory from all linked programs? hselasky: Doesn't this static steal memory from all linked programs? | |||||
Not Done Inline ActionsIt might be more appropriate for the caller to provide a buffer to store the result. cem: It might be more appropriate for the caller to provide a buffer to store the result. | |||||
Not Done Inline ActionsYea, But having that interface, though does require extra copies. imp: Yea,
char *getlocalbase(char *buffer, size_t buflen)
seems better and doesn't suffer the multi… | |||||
Done Inline Actions
That would be a signature much like the one that Scott just reverted. It was much more complex, especially with regard to error handling, and required significantly more complex code in the caller. The statically assigned buffer makes this program thread-safe since all threads see the same environment and the same sysctl values (as you already know).
This interface is an easy drop-in replacement for programs that used to query the environment (for LOCALBASE. se: > Yea,
> char *getlocalbase(char *buffer, size_t buflen)
> seems better and doesn't suffer the… | |||||
Done Inline Actions
See the commit history - a version that did just that has been reverted. se: > It might be more appropriate for the caller to provide a buffer to store the result.
See the… | |||||
Done Inline Actions
Yes, this is a static buffer that will be allocated (once) for programs that link against libutil. se: > Doesn't this static steal memory from all linked programs?
Yes, this is a static buffer that… | |||||
Done Inline Actions
se: > Doesn't this static steal memory from all linked programs?
| |||||
{ | |||||
static const int localbase_oid[2] = {CTL_USER, USER_LOCALBASE}; | |||||
char *tmppath; | |||||
Done Inline ActionsThis should be "static const" hselasky: This should be "static const" | |||||
size_t tmplen; | |||||
static const char *localbase = NULL; | |||||
Done Inline ActionsThis will break the usage of many people and probably the main usage of getenv(LOCALBASE), one of the usage for instance is to build the ports tree locally as a user... bapt: This will break the usage of many people and probably the main usage of getenv(LOCALBASE), one… | |||||
Done Inline Actionsmy bad I mis read the function ;) bapt: my bad I mis read the function ;) | |||||
Done Inline ActionsThe issetugid check is required to prevent privilege escalations and is identical to e.g. the prevention of LD_PRELOAD being used for SUID binaries. se: The issetugid check is required to prevent privilege escalations and is identical to e.g. the… | |||||
if (issetugid() == 0) { | |||||
tmppath = getenv("LOCALBASE"); | |||||
if (tmppath != NULL && tmppath[0] != '\0') | |||||
return (tmppath); | |||||
} | |||||
if (sysctl(localbase_oid, 2, NULL, &tmplen, NULL, 0) == 0 && | |||||
(tmppath = malloc(tmplen)) != NULL && | |||||
sysctl(localbase_oid, 2, tmppath, &tmplen, NULL, 0) == 0) { | |||||
/* | |||||
* 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. | |||||
*/ | |||||
if (tmppath[0] != '\0' && | |||||
(volatile const char*)localbase == NULL) | |||||
localbase = tmppath; | |||||
else | |||||
free((void*)tmppath); | |||||
return (localbase); | |||||
} | |||||
return (_PATH_LOCALBASE); | |||||
} | |||||
Done Inline Actions"static const" here. hselasky: "static const" here. | |||||
Done Inline ActionsOne simple way to avoid issue with races, is to use the constructor keyword on a separate function, that only initializes "localbase". Then localbase only needs to return a pointer. hselasky: One simple way to avoid issue with races, is to use the constructor keyword on a separate… |
style(9):
That is, delete the redundant sys/types include and move sys/param to the top.