gh-143379: fix UAF in struct.Struct.pack() when object modified by dunder methods#143382
gh-143379: fix UAF in struct.Struct.pack() when object modified by dunder methods#143382skirpichev wants to merge 8 commits into
Conversation
… by dunder methods
0bfe0a3 to
a1c2603
Compare
January 3, 2026 07:36
|
Alternative approach: use some flag in PyStructObject struct to forbid mutation during pack(). |
Sorry, something went wrong.
Would it be possible to always disallow mutation? Raise an exception if |
Sorry, something went wrong.
|
Why there is Maybe all logic in Here is the plan:
|
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
Now modification of the Struct() while packing trigger a RuntimeError
|
Ok, new version just disallows mutation of Struct() when s_pack_internal() is running. When support for Edit: see #143643 for next steps. |
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Hmm, indeed.
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. |
Sorry, something went wrong.
|
Ok, this lacks mutex in |
Sorry, something went wrong.
|
Oh, you closed your PR. Why? |
Sorry, something went wrong.
See issue thread. I think that this not solves some practical problem, but introduce code complexity. If we make eventually make Struct immutable - #143379 will be fixed. |
Sorry, something went wrong.
Added a mutex flag to trigger RuntimeError if Struct() modification happens during packing.
s_pack_internalvia re-entrant__bool__#143379