Page MenuHomeFreeBSD

pthread_attr_get_np(): Use malloc(), report ENOMEM, don't tamper on error
ClosedPublic

Authored by olce on Jan 5 2024, 2:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 8:43 PM
Unknown Object (File)
Tue, Nov 5, 3:13 PM
Unknown Object (File)
Thu, Oct 17, 10:21 AM
Unknown Object (File)
Wed, Oct 16, 3:07 PM
Unknown Object (File)
Oct 14 2024, 10:43 PM
Unknown Object (File)
Oct 14 2024, 10:43 PM
Unknown Object (File)
Oct 13 2024, 5:52 PM
Unknown Object (File)
Oct 12 2024, 9:02 PM
Subscribers

Details

Summary

Similarly as in the previous commit, using calloc() instead of malloc()
is useless here in the regular case since the subsequent call to
cpuset_getaffinify() is going to completely fill the allocated memory.

However, there is an additional complication. This function tries to
allocate memory to hold the cpuset if it previously wasn't, and does so
before the thread lock is acquired, which can fail on a bad thread ID.
In this case, it is necessary to deallocate the memory allocated in this
function so that the attributes object appears unmodified to the caller
when an error is returned. Without this, a subsequent call to
pthread_attr_getaffinity_np() would expose uninitialized memory (not
a security problem per se, since it comes from the same process) instead
of returning a full mask as it would before the failing call to
pthread_attr_get_np(). So the caller would be able to notice a change
in the state of the attributes object even if pthread_attr_get_np()
reported failure, which would be quite surprising. A similar problem
that could occur on failure of cpuset_setaffinity() has been fixed.

Finally, we shall always report memory allocation failure. This already
goes for pthread_attr_init(), so, if for nothing else, just be
consistent.

MFC after: 2 weeks

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

olce requested review of this revision.Jan 5 2024, 2:18 PM
This revision is now accepted and ready to land.Jan 5 2024, 4:09 PM

A slight difference to the previous commit though is that we may return with an error (if _thr_find_thread fails) leaving dst->cpuset pointing to uninitialized memory

lib/libthr/thread/thr_attr.c
150

hmm, should we free dst->cpuset in the error case (and set it to NULL)?

olce marked an inline comment as done.Jan 5 2024, 5:29 PM
olce added inline comments.
lib/libthr/thread/thr_attr.c
150

That's a very good question, which I also asked to myself. All the other uses of cpuset in the file check whether it is NULL, and if it is, allocate a new area, except for pthread_attr_getaffinity_np() whose purpose is to output what the already allocated area contains.

I initially noticed that not freeing wasn't a problem since the allocated area is anyway freed when calling pthread_attr_destroy(), from a memory leak point of view.

This analysis didn't take into account however the case where pthread_attr_get_np() is called with a bad ID, allocates some memory but doesn't initialize it, and then the caller calls pthread_attr_getaffinity_np() and gets uninitialized garbage. This is not a security problem per se (since this garbage comes from the same process), and I'm not sure we should go further than the "garbage in, garbage out" policy. But if we do, alternatively, free() could be called on error, or, e.g., the memory zeroed. Or, even "better", the function would free() only in the case where it actually allocated the memory. These considerations may have been the reason for the original calloc() call. What do you think? Personally, I wouldn't bother.

olce marked an inline comment as done.Jan 5 2024, 5:38 PM
olce added inline comments.
lib/libthr/thread/thr_attr.c
150

I changed my mind... Going to implement the "better" solution. It's the only one guaranteeing that the attributes object is visibly unmodified after a call to pthread_attr_get_np() that returns an error. I think this is the only behavior that actually passes POLA.

olce planned changes to this revision.Jan 5 2024, 9:12 PM

Implement the "better" solution.

This includes another fix for the case where cpuset_setaffinity() fails in order to ensure that the attributes object is never modified if pthread_attr_get_np() reports failure.

lib/libthr/thread/thr_attr.c
151

and this check is really not needed.

lib/libthr/thread/thr_attr.c
151

I think you're mistaken. Contrast dst->cpuset and cpuset. The original code is correct.

olce marked an inline comment as done.Jan 9 2024, 1:37 PM
lib/libthr/thread/thr_attr.c
160

Ok, but then wouldn't you leak cpuset there?

Why not assign dst->cpuset outright?

olce retitled this revision from libthr: pthread_attr_get_np(): calloc() -> malloc(), report ENOMEM to pthread_attr_get_np(): Use malloc(), report ENOMEM, don't tamper on error.
olce edited the summary of this revision. (Show Details)

Address the leak of cpuset.

Update the commit message (had been updated previously, but I hadn't uploaded it to Phabricator).

lib/libthr/thread/thr_attr.c
160

Ok, but then wouldn't you leak cpuset there?

Oh yes, you're right, my bad. Fixed.

Why not assign dst->cpuset outright?

Because of the principle that we don't touch dst in any way until we know we can't fail, so that the caller sees an unmodified output on error. If I changed dst->cpuset before the call to cpuset_getaffinity() and the latter would fail, the user could notice the change and read newly allocated garbage through a subsequent call to pthread_attr_get_affinity_np().

olce marked an inline comment as done.Jan 9 2024, 2:02 PM
lib/libthr/thread/thr_attr.c
163

Why is this block removed?

167

Or rather, since you removed the memcpy() block and now dst->flags is copied from the pthread itself, dst->flags gets polluted with internal bits.

kib added inline comments.
lib/libthr/thread/thr_attr.c
167

Hm, it was contaminated before as well.

This revision is now accepted and ready to land.Jan 9 2024, 5:06 PM
olce marked 3 inline comments as done.Jan 9 2024, 5:44 PM
olce added inline comments.
lib/libthr/thread/thr_attr.c
163

Because the double copy was unnecessary, and everything is now copied into *dst directly (once it is known that the function can't fail).

167

Indeed. I've not checked that part further than ensuring that the existing behavior was preserved. Which internal bits did you have in mind? If it's just a matter of filtering them out, we could do it as a quick and small follow-up.