Date: Wed, 18 Nov 2020 23:11:56 +0100 From: Stefan Esser <se@freebsd.org> To: Jessica Clarke <jrtc27@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r367813 - head/lib/libutil Message-ID: <0d844881-b3dd-a448-f33b-8268f40a4c94@freebsd.org> In-Reply-To: <E6A6DAB2-BB3D-497D-AF53-CAF958AD2C5E@freebsd.org> References: <202011181944.0AIJiUU3003699@repo.freebsd.org> <25465269-5497-4981-A1E4-CC1FFAB68CF4@freebsd.org> <E6A6DAB2-BB3D-497D-AF53-CAF958AD2C5E@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--qMYyOR5Qn8FrF2dfMjfRouXT32PvaGURb
Content-Type: multipart/mixed; boundary="DHNTWTyb9XTzxIjO2X0W29YXL5HHvsDmj";
protected-headers="v1"
From: Stefan Esser <se@freebsd.org>
To: Jessica Clarke <jrtc27@freebsd.org>
Cc: src-committers <src-committers@freebsd.org>,
svn-src-all <svn-src-all@freebsd.org>, svn-src-head@freebsd.org
Message-ID: <0d844881-b3dd-a448-f33b-8268f40a4c94@freebsd.org>
Subject: Re: svn commit: r367813 - head/lib/libutil
References: <202011181944.0AIJiUU3003699@repo.freebsd.org>
<25465269-5497-4981-A1E4-CC1FFAB68CF4@freebsd.org>
<E6A6DAB2-BB3D-497D-AF53-CAF958AD2C5E@freebsd.org>
In-Reply-To: <E6A6DAB2-BB3D-497D-AF53-CAF958AD2C5E@freebsd.org>
--DHNTWTyb9XTzxIjO2X0W29YXL5HHvsDmj
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Am 18.11.20 um 22:27 schrieb Jessica Clarke:
> On 18 Nov 2020, at 21:15, Jessica Clarke <jrtc27@freebsd.org> wrote:
>>
>> On 18 Nov 2020, at 19:44, Stefan E=C3=9Fer <se@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?
>=20
> 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 =3D malloc(tmplen)) !=3D NULL &&
sysctl(localbase_oid, 2, tmppath, &tmplen, NULL, 0) =3D=3D 0=
) {
if (tmppath[0] !=3D '\0' &&
(volatile const char*)localbase =3D=3D NULL)
localbase =3D tmppath;
else
free((void*)tmppath);
return (localbase);
If localbase =3D=3D NULL then localbase is set to tmppath, but another co=
re
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
--DHNTWTyb9XTzxIjO2X0W29YXL5HHvsDmj--
--qMYyOR5Qn8FrF2dfMjfRouXT32PvaGURb
Content-Type: application/pgp-signature; name="OpenPGP_signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="OpenPGP_signature"
-----BEGIN PGP SIGNATURE-----
wsB5BAABCAAjFiEEo3HqZZwL7MgrcVMTR+u171r99UQFAl+1nCwFAwAAAAAACgkQR+u171r99UQY
EAf/aY0J0wmk2kElUAn4URBTFS3jWv7CRDhBI2l/RwRu3MzN+kIPCzXisBjI5A/JZbbkkw5aGOvX
3vwbrO0Od2McUYRnpe6lMxfiGBDbq9jAsZNFc9T1c3ZgCUpOHon/CP0c6AvKNaZDtNEJy8OFDdOz
7NHpTjW+sEFB/iX/F+TYxXJThzbkiMqf76+DqLhULrNY1qYWV5ZYXSJdh5IJFJd3GcZkLrZoosRv
SYDqDOs+TZ1sKPW/tyId+YQjY5hQVwnPRyZkMHz1vlYSmNjIxq+Ui+FWzs8RM+arW9LnpxmiGIQ5
qTt9vKgzGtxkb6O+fhUtHP1A5kyVxjAIzvtUlObouw==
=8FYQ
-----END PGP SIGNATURE-----
--qMYyOR5Qn8FrF2dfMjfRouXT32PvaGURb--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0d844881-b3dd-a448-f33b-8268f40a4c94>