-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-40705: Fix use-after-free in _zoneinfo's module_free #20280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for the report and the PR. I am mildly uncomfortable with this way of solving the issue, because it seems like both the "module is created two times" and the "someone manages to use one of these globals after it is free" code paths are basically theoretical, and in my judgement the "module exists twice" is much more likely to occur in the near future. Since, as far as I can tell, this is not exploitable, I'd prefer to take a little time to come to a solution for both, if possible. That said, I don't really understand how this can cause use-after-free, so I am having trouble coming up with alternate solutions. Would this work? if (TIMEDELTA_CACHE != NULL && Py_REFCNT(TIMEDELTA_CACHE) == 1) {
Py_CLEAR(TIMEDELTA_CACHE);
} else {
Py_XDECREF(TIMEDELTA_CACHE);
} We'd need to do some locking if we ever get to a state where the GIL is removed or where there are additional GILs, but that's not the current state of things and even if it were the failure mode is a small memory leak in very unusual circumstances rather than any sort of crash. |
The caches are static globals. Is it safe to mix multi-phase initialization with globals? I was under the impression that multi-phase initialization and multi-module instances require per-module state so objects cannot leak between subinterpreters. |
Yeah, that solution would work. Essentially what's happening is that there's only one instance of I think @tiran is right though, if you want to account for multiple modules then this cache needs to be at the module level instead of a global. See the example of posixmodule: Lines 816 to 844 in a487a39
|
Yeah, this code is essentially a weird intermediate stage where it has the form of multi-phase initialization, but it's not actually safe to use with multi-module instances. I gave storing the caches on the module a try but it was annoying complicated and not important anyway since I think that when I originally wrote this, I needed PEP 573 to properly implement this in a subinterpreter-safe way, but I was targeting Python 3.8, because the reference implementation was destined to become a backport. Now that it's in the standard library we can migrate over to using module state (assuming we can do it without major performance regressions), but I think we'll have to target 3.10 for that (unless big refactors like that are allowed during the beta period).
Ah, that's pretty obvious now that you say it. In that case, then yes I think we should use the conditional, since I believe that — despite the fact that nothing else will ever have a reference to this at the moment — everything else assumes that these are ref-counted and that more than one reference to it can exist. |
At first glance it looked easy to port the new module to PEP 489 multi-phase init. After I found more than eight globals I pretty much gave up. The endeavor turned out to be a lengthy and dull task. I didn't even realize that datetime needs to be fixed first! How about use use the safe singleton API
|
f5bc283
to
296ec86
Compare
I updated the diff to be the short minimal fix that accounts for multiple modules for now, one of the branches won't ever be taken right now. We could go with the singleton module approach and remove the in-limbo code for 3.9 but that might make it harder to back-port stuff from 3.10 in the future. |
Thanks @ammaraskar for the PR, and @pganssle for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
GH-20319 is a backport of this pull request to the 3.9 branch. |
) (cherry picked from commit 06a1b89) Co-authored-by: Ammar Askar <[email protected]>
TIMEDELTA_CACHE = NULL; | ||
if (TIMEDELTA_CACHE != NULL && Py_REFCNT(TIMEDELTA_CACHE) > 1) { | ||
Py_DECREF(TIMEDELTA_CACHE); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, I just realize this is a PEP 7 violation, it should be:
if (TIMEDELTA_CACHE != NULL && Py_REFCNT(TIMEDELTA_CACHE) > 1) {
Py_DECREF(TIMEDELTA_CACHE);
}
else {
Py_CLEAR(TIMEDELTA_CACHE);
}
Since we're probably going to re-write this code soon-ish anyway, I guess we'll just leave it here and in the backport until someone touches it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah sorry about that :(
Interestingly, it seems like it's the |
Huh, that's weird. Even in that situation I would expect the other branch to be hit at least once, when the last reference to the module is lost. |
The one on master shows the opposite coverage: https://codecov.io/gh/python/cpython/src/master/Modules/_zoneinfo.c#L2607 |
@ammaraskar I think the difference is that because of the different module organization ( Either way, I'm kinda glad we did it this way after all, because it seems likely that semi-insignificant refactoring could end up leading to segfaults in some situations. I'm going to merge the backport. |
(cherry picked from commit 06a1b89) Co-authored-by: Ammar Askar <[email protected]>
initialize_caches
currently seems to be designed to account for being called twice but it's only callee is thePy_mod_exec
. This might have been future planning for heap allocated types but I'll wait for a response from @pganssle on their intentions with this code there. Until then this seems like a straight-forward fix.Couldn't really think of an easy way to test this since it doesn't seem like there's a codepath where a reference to
TIMEDELTA_CACHE
orZONEINFO_WEAK_CACHE
could be held by someone outside the module.https://bugs.python.org/issue40705