[Memory error/Bad free] Objects/setobject.c, PyMem_Free may free local variable. Found by Clang Static Analyzer(CSA)

[wr-web]

Bug report

Bug description:

Objects/setobject.c
small_copy is a local variable

    setentry small_copy[PySet_MINSIZE];

In some situation, small_copy will be passed to oldtable

            /* We're not going to resize it, but rebuild the
               table anyway to purge old dummy entries.
               Subtle:  This is *necessary* if fill==size,
               as set_lookkey needs at least one virgin slot to
               terminate failing searches.  If fill < size, it's
               merely desirable, as dummies slow searches. */
            assert(so->fill > so->used);
            memcpy(small_copy, oldtable, sizeof(small_copy));
            oldtable = small_copy;

oldtable may be badly freed

    if (is_oldtable_malloced)
        PyMem_Free(oldtable);
    return 0;

More concrete report packed in report.zip.
File list:

• report-316784.html
• scanview.css
• sorttable.js

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

[wr-web]

For zip is not allowed to upload, I pasted part of report image here

[rhettinger]

The line PyMem_Free(oldtable) can only be reached if is_oldtable_malloced is True. That happens when oldtable != so->smalltable.

The line oldtable = small_copy can only be reached if newtable == oldtable which is equivalent to oldtable == so->smalltable.

Since oldtable != so->smalltable and oldtable == so->smalltable are mutually exclusive, the PyMem_Free(oldtable) and oldtable = small_copy lines are mutually exclusive as well.

[ZeroIntensity]

I looked through most of the open type-crash issues, and there's nothing about set ever segfaulting, so yeah, I doubt this ever occurs in practice. This code was added nearly 20 years ago by @rhettinger, so it's been holding up for quite a while without problems.

If this is somehow an actual thing that can occur, it must be an extremely tight edge case. It's very likely a false positive with clang's static analyzer, and could be worth reporting to them.

(@encukou, this needs pending. Someone will probably do it for you, though 😄)

[wr-web]

Thank you for your contribution to the open-source community! I think this is most likely a false positive, probably due to the static analysis tool CSA not handling the constraints properly. As @rhettinger mentioned, the contradiction lies in oldtable != so->smalltable versus oldtable == so->smalltable. Additionally, I thought that in a multithreading/multiprocessing scenario, this could potentially lead to a crash. Before reaching newtable = so->smalltable, if so->smalltable is changed to be equal to oldtable, it could still be triggered, though this would be an extremely strict corner case.

[vstinner]

I think this is most likely a false positive, probably due to the static analysis tool CSA not handling the constraints properly.

Correct, it's a false positive. Look at the code and what @rhettinger explained: it cannot happen.

I close the issue.