On Tue, 26 Dec 2023 at 07:32, Sam Varshavchik <mr...@courier-mta.com> 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 char * lastUname = NULL; // So lastUname is NULL
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);
commit fe645f822dbd71da4145f6174e526a09eb5c815e
Author: Panu Matilainen <pmati...@redhat.com>
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.c
rpmio/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@lists.fedoraproject.org
To unsubscribe send an email to devel-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
Do not reply to spam, report it:
https://pagure.io/fedora-infrastructure/new_issue
