svn commit: r367813 - head/lib/libutil

[Stefan Esser]

Am 18.11.20 um 22:27 schrieb Jessica Clarke:
> On 18 Nov 2020, at 21:15, Jessica Clarke <jrtc27 at freebsd.org> wrote:
>>
>> On 18 Nov 2020, at 19:44, Stefan Eßer <se at 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?
>>
>> What happens if I set the value with a sysctl and call this?
> 
> Notably, you go to all this trouble to have a localbase variable that
> gets set, but you never actually use it properly as a cache since you
> do the full lookup and only then realise that you already had a
> (possibly stale) value cached.

Do you want to suggest a better implementation?

As explained in another mail, this case should never happen, since
the function is not called in a multi-threaded context and if it
was, then the test before the assignment reduces the time window
of vulnerability to a few nanoseconds, and if a collision did
occur the amount of heap space lost is negligible.

Or do you think, that the assignment should directly go to the
localbase variable, not to tmppath? That would significantly
enlarge the window of vulnerability, and the code that protects
against this case (though not perfectly) seems worth it.

To give some context (slightly simplified):

         if (tmppath = malloc(tmplen)) != NULL &&
             sysctl(localbase_oid, 2, tmppath, &tmplen, NULL, 0) == 0) {
                 if (tmppath[0] != '\0' &&
                     (volatile const char*)localbase == NULL)
                         localbase = tmppath;
                 else
                         free((void*)tmppath);
                 return (localbase);

If localbase == NULL then localbase is set to tmppath, but another core
could execute exactly the same instructions in the same few nanoseconds.

The assignment could also have been marked volatile to force the write
to memory, but since this is a library function called from another
compile unit and the value is returned into that other context, I think
the write will happen immediately, anyway.

I know about architectures with non-coherent caches and other issues
that could increase the time window, but as said before, I do not expect
this function to be called in a multi-threaded context, but thought the
protection against a collision in the much larger time window covering
the malloc and sysctl system call might still be worth the 1 extra line
of code required to go through a stack allocated tmppath instead of
directly assigning to localbase.

Regards, STefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20201118/bd8ff5a9/attachment.sig>