On Thu, Nov 19, 2020 at 12:05:51AM +0100, Stefan Esser wrote: > Am 18.11.20 um 23:14 schrieb Jessica Clarke: > > On 18 Nov 2020, at 21:52, Stefan Esser <s...@freebsd.org> wrote: > >> Am 18.11.20 um 22:15 schrieb Jessica Clarke: > >>> On 18 Nov 2020, at 19:44, Stefan E??er <s...@freebsd.org> wrote: > >>>> + /* > >>>> + * 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. > >>>> + */ > >>> Why was this committed with a known racy/leaky implementation? > >> > >> Because the alternatives that I offered for discussion were > >> less acceptable. > > > > That has no bearing over whether this one is. > > Three alternatives have been discussed: > > 1) static buffer of size MAXPATHLEN > 2) dynamically allocated buffer (as committed) > 3) dynamically allocated buffer filled by a constructor > > 1) has been rejected, since it adds 1 KB of bss to each program > that is linked with libutil, whether it uses getlocalbase() or > not. > > 3) has been rejected since it causes 1 getenv() and 2 sysctl() > calls when the program linked with libutil starts, independently > of whether getlocalbase() is used or not. > > 2) has been selected, since it is only called when needed and it > does not pre-allocate a large buffer. > > Which other alternative do you want to suggest? > > Did you have a look at the review announced on the -current list > and mentioned in the commit? > > >>> What happens if I set the value with a sysctl and call this? > >> > >> You'll get the value set with sysctl, unless overridden by the > >> environment variable. There is a window of a few nano-seconds > >> where a thread executing in parallel on another core might be > >> able to set the localbase variable (between the test for NULL > >> in this function and the assignment to it). The value that will > >> be returned by either thread will be identical, so there is no > >> risk of corruption of the result. > > > > But if I call getlocalbase, then set it via sysctl, then call > > getlocalbase again, I see the old value. If, however, I omit the first > > getlocalbase, I see the new value. This differs from how getenv/setenv > > of the value work, where you always see the up-to-date value. Maybe you > > think that's a feature, but it's something to watch out for and > > explicitly call out in the documentation. > > Actual programs call getlocalbase() exactly once and create the > required pathes from the value returned. > > It is possible to copy the value from the environment to a buffer, > but at added complexity and introducing another race condition. > > The program itself might have modified its environment and then > use an inconsistent value when it calls getlocalbase() again > thereafter. But why would you want to do this - it seems to be > a complicated way lof foot-shooting to me. > > I'm not a native speaker of English and not best qualified to > clearly express this in the man-page. Feel free to commit any > clarification or suggest an addition for me to commit. > > > You also misunderstand all the subtleties of multithreading here. There > > are no acquire/release pairs so it is entirely legal for Thread 2 to > > read Thread 1's initialised value for localbase before the contents of > > it are visible (i.e. the pointer is initialised but the data is > > garbage). > > Yes, and thread 2's value will be identical to the one from > thread 1, just in a different heap location, unless there is a > write to the sysctl variable in the kernel at just the same > time. And you cannot protect against this race and you'll get > either the old or new value. > > > The `(volatile const char*)localbase` cast is also a complete waste of > > time. You probably meant to write `(const char * volatile)localbase` > > but even then that does nothing useful as the cast is too late. What > > you really were trying to write was > > `*(const char * volatile *)&localbase`, but you need proper atomics > > anyway for this to be safe. > > I was not sure about the correct volatile declaration, I've got > to admit. It was in the review and I did not get any comments > regarding that specific modifier. The goal was to enforce the > access to the localbase pointer in memory after returning from > the sysctl to shorten the window. > > Perhaps I should just have completely ignore any multi-threading > issues and accepted that an overlapping execution of this function > would allocate multiple sysctl destination buffers but loose all > references but one in the assignment to localbase. > > It will not happen in practice, it just does not make sense to > call this function more than once in a program. > > >> This unlikely case may actually leak a heap allocated string > >> of typically tens of bytes (but with negligible probability). > >> > >> But this really is a non-issue, since there should never be a > >> reason to invoke this function in a multi-threaded context. > > > > 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. > And unless the program modifies its own environment at run-time, > all invocsations of getlocalbase() are guaranteed to return the > same result again and again. > > > Also if malloc returns NULL I would quite like that to be an error for > > the function and not silently fall back on _PATH_LOCALBASE. > > The function is specifically specified to always succeed. Else > there is extra code required at each invocation to check for > errors. Not being able to allocate the required buffer of at > most 1 KB will cause every non-trivial program to fail anyway. > > Returning the compiled in default value will direct the program > to access files in /usr/local, which is an administrator controlled > path, even on systems with non-default localbase. > > Returning NULL is possible in that case, but the only ways a > program could deal with this case is to either crash, sue the > default path, or ignore the accesses to files in localbase. > > In either case it will not operate as intended, but I consider > directing accesses to the FreeBSD default location (or the > modified _PATH_LOCALBASE) to be a reasonable fall-back. > > But this will not happen unless provoked (by not allowing the > program to allocate any memory from the heap, which will cause > it to fail as soon as any other library function or start-up > code tries to allocate heap-space). > > I had put this code up for review and it has been discussed > before (and another implementation has been reverted after > several iterations of changes). > > If there had been consensus to return NULL, I could have > committed that - it is a trivial change and ENOMEM would have > been set after a malloc failure. > > 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.) -- Brooks
signature.asc
Description: PGP signature
