Re: rpmbuild core dumps

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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 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 <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.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@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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Fedora Announce]     [Fedora Users]     [Fedora Kernel]     [Fedora Testing]     [Fedora Formulas]     [Fedora PHP Devel]     [Kernel Development]     [Fedora Legacy]     [Fedora Maintainers]     [Fedora Desktop]     [PAM]     [Red Hat Development]     [Gimp]     [Yosemite News]

  Powered by Linux