Am 18.11.20 um 22:40 schrieb Mateusz Guzik:
+{ + static const int localbase_oid[2] = {CTL_USER, USER_LOCALBASE};There is no use for this to be static.
Why not? This makes it part of the constant initialized data of the library, which is more efficient under run-time and memory space aspects than initializing them on the stack. What do I miss?
+ char *tmppath; + size_t tmplen; + static const char *localbase = NULL; + + 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) {Apart from the concurrency issue mentioned in the comment this is just very wasteful. Instead you can have a small local buffer, say 128 bytes and pass that to be populated. The sysctl handler than can populate that and return an error if the size is too small. I don't know if sysclt api allows it to return the set size as it is. Worst case you can just retry with a bigger malloced buffer.
You obviously did not follow the development of this code in the review. It used to have such a buffer, but this was rejected, since only very few of the programs that link against this library are going to call this function. Allocating a small buffer and replacing it with a larger one if it was too small adds needless complexity to this program and needs more code. It is not sensible (IMHO) to reduce the number of system calls by 1 for the small number of programs that use this feature, and which perform at least tens of other system calls at start-up.
Once you get the result you can malloc a buffer and atomic_cmpset_rel_ptr localbase to point to it. If this fails, another thread got the result, you free your buffer and return (localbase).
Yes, I wrote that I could use atomics, feel free to modify the program accordingly. It will not make a difference in practice, since there is no good reason to call this function from within a multi-threaded context at all, and none of the candidates to be modified to use this function in base do.
Also the kernel counter part completely unnecessarily comes with a static 1KB buffer to hold what typically is going to be nothing. This should also be allocated as needed. Worst case you can add a trivial lock around it.
The kernel buffer does already exist and you did not complain about that buffer, when it has been committed. A size of PATHNAMEMAX is used to allow for any valid path name to be set from the loader. If there is consensus that the value should be limited to e.g. 64 or 128 bytes, this is trivially changed in kern_mib.c. I'd be more worried, if this buffer existed not as a single instance in the kernel but e.g. per process, but this is not the case. Feel free to make this a dynamically allocated variable, but make sure that you do not increase the code size by as much as you reduce the static storage required. I do not need the dynamic assignment to localbase at all, and I have suggested to introduce a kernel build option (e.g. WITH_LOCALBASE_VAR) to control compilation of kernel and library with this function or to return just the constant string _PATH_LOCALBASE or "/usr/local". But there were very strong requests for this dynamically set localbase and I have provided an implementation that is functional and easy to use. Regards, STefan
OpenPGP_signature
Description: OpenPGP digital signature
