gh-143379: fix UAF in struct.Struct.pack() when object modified by dunder methods

[skirpichev]

Added a mutex flag to trigger RuntimeError if Struct() modification happens during packing.

• Issue: Use-after-free in s_pack_internal via re-entrant __bool__ #143379

[skirpichev]

Alternative approach: use some flag in PyStructObject struct to forbid mutation during pack().

[vstinner]

Alternative approach: use some flag in PyStructObject struct to forbid mutation during pack().

Would it be possible to always disallow mutation? Raise an exception if __init__() is called twice.

[skirpichev]

Why there is __init__() method at all? The Struct expected to be an immutable type.

Maybe all logic in Struct___init__ (tp_init) should be moved to s_new (tp_new)? I think this will fix issue. Looking in the history, the custom __init__() was re-introduced back in #112358.

Here is the plan:

1. create a flag to temporary forbid mutation
2. deprecate Struct.__new__() calls without one required argument
3. eventually drop Struct.__init__() together with a flag

CC @serhiy-storchaka

[serhiy-storchaka]

This is too complicated and expensive.

Would not it be easier to forbid calling __init__() more than once? This can break some weird code, so as an intermediate solution we can add a mutex flag which forbids calling __init__() when the object is used.

[serhiy-storchaka]

Seen #143382 (comment) after trying to review the code. I agree with @skirpichev, a flag is the right temporary solution, and we should think about getting rid of __init__() in future.

[skirpichev]

Ok, new version just disallows mutation of Struct() when s_pack_internal() is running. When support for __init__() will be dropped - the mutex field can be removed.

Edit: see #143643 for next steps.

[serhiy-storchaka]

Would not it prevent concurrent use of pack()? It would be undesirable. We need a counter (atomic for GIL-less build) which would allow concurrent operations, but block __init__(). Please test how it affects performance.

To be absolutely safe, we would need also a mutex which would block packing while __init__() is executed. Because there is a race condition between checking if it is safe to modify the struct state and modifying it. But it is very unlikely to happen in real world (why would anybody call __init__() concurrently with pack()?), so we can ignore this for now. Well, ignoring this issue until we forbid repeated calls of __init__() is also solution.

[skirpichev]

Would not it prevent concurrent use of pack()?

Hmm, indeed.

Well, ignoring this issue until we forbid repeated calls of init() is also solution.

We can do this after a deprecation (see ongoing work in #143643). But then this will be a non-issue anymore. Then, maybe we can close the #143379 as a duplicate of #78724? The real problem here is that Struct()'s aren't immutable.

[skirpichev]

Ok, this lacks mutex in __init__() and benchmarks. But I'll not push things further. Thanks for review.