On Tue, 26 Dec 2023 at 07:32, Sam Varshavchik <mrsam@xxxxxxxxxxxxxxx> wrote:
Stephen Smoogen writes:
>
> I am guessing the problem is really with the free(lastUname) since the rfree
Yes. Multiple execution threads will reach lastUname and try to free the
same pointer. glibc rightfully complains about the double-free.
I am trying to figure out the logic of this section:
```
static uid_t lastUid;
if (!thisUname) {
lastUname = rfree(lastUname); // lastUname should still be NULL and we are freeing NULL and setting itself back to NULL.
return -1;
```
I expect this is where I am not understanding something basic in C from too many years in non-pointer land. I looked at the change of these lines and they date back to this commit.
fe645f822d (Panu Matilainen 2023-05-04 11:59:36 +0300 136) lastUname = rfree(lastUname);
Author: Panu Matilainen <pmatilai@xxxxxxxxxx>
Date: Thu May 4 11:59:36 2023 +0300
Simplify rpmug caching
The simple cache whose efficiency troubled ewt back in 1997
(see commit 97999ce92c1cad3315d85c02bb3c62007a75d846)
has proven more than adequate over the years.
In a local testcase based on Fedora 33 server iso contents, an install
of 1765 packages consisting of 201344 files did a whopping 27 user +
groups combined. So a few more alloc+free is not going to make the
damnest difference, don't bother with reallocing the cache buffer, just
strdup() a new one when needed.
And the code before that was gnarlier from days of yore.
> isn't referred to (but not sure if an optimization would have removed it. The
> comment before this code mentions that this is a hack to try and get things
> done.. probably from long long ago when rpm was single threaded.
The problem is in all of these functions. It's the same problem with all of
them. Here's rpmugUname(), for example. You have two execution threads
traversing that nest of "if" statements and all of them winding up here:
} else {
char *uname = NULL;
if (lookup_str(pwfile(), uid, 2, 0, &uname))
return NULL;
lastUid = uid;
free(lastUname);
And now both execution threads will try to free() the same pointer.
The next statement resets lastUname to the newly-allocated uname, but it's
too late. If the code that executes in parallel calls rpmugUname, then just
say good night.
All of the static variables in all of the functions here must have a mutex
wrapped around them.
Or declared with a __thread attribute.
The window of vulnerability is very tiny. But I have 32 cores and have 32
execution threads churning. They have about a 5% chance of hitting the
double-free on every build. Worse, I can see how this race condition may not
result in a crash but produce a corrupted rpm.
That is ugly. The only mention of mutexes I found
lib/package.c
rpmio/macro.crpmio/rpmlog.c
The entries for __thread I found come in around 2019. Did you have a bugzilla or a report on https://github.com/rpm-software-management/rpm/ which I can add anything I find?
and most date back from 2013.
-- Stephen Smoogen, Red Hat Automotive
Let us be kind to one another, for most of us are fighting a hard battle. -- Ian MacClaren-- _______________________________________________ devel mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
