| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
When ExternalEntityParserCreate() hits an error path (allocation failure), Py_DECREF(new_parser) triggers xmlparse_dealloc() on a partially-initialized object: 1. handlers is NULL, so clear_handlers dereferences NULL (SEGV). 2. Py_CLEAR(parent) in dealloc already decrements the parent's refcount, so the explicit Py_DECREF(self) is a double-decrement. Fix by adding a NULL guard in clear_handlers and setting parent to NULL before Py_DECREF(new_parser) in each error path so that dealloc does not over-decrement the parent's refcount.
|
@hugovk What's our policy about fixing security issues? should we backport them as far as the original security fix went? |
Sorry, something went wrong.
|
@picnixz very interesting question! I asked that question to myself as well one time in #144118 where it was indirectly answered because this was only backported to the versions that still recieve bug fixes and not the ones which recieve security updates, but I'm not quite sure whether this is the best solution. And maybe my comment just wasn't seen anymore, but this would at least be consistent. So if this is also backported, I suggest that #144118 should be as well. |
Sorry, something went wrong.
|
I hope this helps a little bit! 👍 |
Sorry, something went wrong.
"And one other question: How do we handle this concerning backports? Theoretically this is a bug fix, so it should be ported back to only a few supported versions (those recieving bug fixes and not those only recieving security updates) while this seems kind of interesting to me because the other patch (the denial of these chars) was a security update and for this reason applied backward also to those versions only getting security updates and this included this kind of "bug" and for this reason a "old version" now has a new bug through a new update which is not any more patched if we don't apply this to the ones only recieving security updates which is maybe a little bit a of a problem because those using older versions probably do this because they want them to be stable and in this case we "added a bug" / removed a feature and we won't apply it back to them. So will we also merge this to 3.10 and so on?" was my original comment on the other issue (so that you don't have to search for this) |
Sorry, something went wrong.
|
This is a bit different. I agree that the change with wsgiref should have been backport as much as possible because it's breaking older versions more than it should have. Here, the fix is not that important because it's anyway in an error path and the worst we can have is likely just a crash. But this only occurs when we don't have enough memory. Crashing is usually not good, but assessing whether something should be considered a security fix is really on a case-by-case basis and left at the discretion of core developers and the PSRT. So, for this specific PR, I don't mind if it's not backported prior to 3.13. For your other PR, I'm going to raise your concers on the issue directly (which was closed before the backports were done). |
Sorry, something went wrong.
|
I agree that whether this should be considered a security fix depends on whether the issue is exploitable and has meaningful security impact, as mentioned. If it is not exploitable and only results in a crash in an error path, then not backporting it to security-only branches seems reasonable. Why is PSRT delaying the response? (@picnixz ) |
Sorry, something went wrong.
Thank you! |
Sorry, something went wrong.
They are not delaying the response. My mention to the PSRT was essentially to indicate that decisions about security issues is the responsibility of maintainers and the PSRT, contributors (they may contribute to the discussion but eventually maintainers decide whether or not to consider it a security issue). Now, as I said, this is not really a security issue here. The original issue was more important (that is, subparsers outliving their parents and the original crash could be reproduced easily with pure Python code), but this specific one is really in error paths so it's hard to exploit as it requires a memory allocation failure. If memory allocation fails usually this means that we already in a bad environment, and other pieces of the program could also be affected. |
Sorry, something went wrong.
If it's just a bugfix, only to bugfix branches. If there's a problem with the original security fix such that it's still exploitable, and the bugfix is also needed to make it work, then it can probably go back to security branches. See https://devguide.python.org/developer-workflow/development-cycle/#security-branches. |
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
|
What is next step ? @picnixz |
Sorry, something went wrong.
|
I'll merge it once I have time (likely this we). My daily job is taking me most of my bandwidth unfortunately. |
Sorry, something went wrong.
|
Thanks a lot!I really appreciate your time and effort, especially with your daily workload. |
Sorry, something went wrong.
|
I'm sorry I forgot!! |
Sorry, something went wrong.
|
Sorry, @raminfp and @picnixz, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker e6b9a1406980fbb1d4032eca9cc0b4f8f252b716 3.13 |
Sorry, something went wrong.
…ror paths (pythonGH-144992) (cherry picked from commit e6b9a14) Co-authored-by: Ramin Farajpour Cami <ramin.blackhat@gmail.com> Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
GH-146019 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
@raminfp Can you do the 3.13 backport please? |
Sorry, something went wrong.
|
GH-146142 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
…rror paths (GH-144992) (#146142) * gh-144984: Fix crash in Expat's `ExternalEntityParserCreate` error paths (#144992) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> * gh-144984: Skip test under tracerefs (GH-146218) --------- Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Petr Viktorin <encukou@gmail.com>
…ror paths (python#144992) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
| Back | FazBrowse Home | New Git URL |
Fix crash when ExternalEntityParserCreate() hits an error path
(allocation failure). Py_DECREF(new_parser) calls xmlparse_dealloc()
on a partially-initialized object where handlers is NULL, causing a
NULL pointer dereference in clear_handlers. Additionally,
Py_CLEAR(parent) in dealloc already decrements the parent's refcount,
making the subsequent Py_DECREF(self) a double-decrement.