Skip to content

Conversation

@hartwork
Copy link
Contributor

@hartworkhartwork commented Oct 3, 2025

Expose the XML Expat 2.7.2 mitigation APIs to disallow use of disproportional amounts of dynamic memory from within an Expat parser (see CVE-2025-59375 for instance).

The exposed APIs are available on Expat parsers, that is, parsers created by xml.parsers.expat.ParserCreate(), as:

  • parser.SetAllocTrackerActivationThreshold(threshold), and
  • parser.SetAllocTrackerMaximumAmplification(max_factor).

(cherry picked from commits f04bea4 and 68a1778)

CC @picnixz

picnixzand others added 4 commits October 3, 2025 01:42
…2025-59375) (python#139234) Expose the XML Expat 2.7.2 mitigation APIs to disallow use of disproportional amounts of dynamic memory from within an Expat parser (see CVE-2025-59375 for instance). The exposed APIs are available on Expat parsers, that is, parsers created by `xml.parsers.expat.ParserCreate()`, as: - `parser.SetAllocTrackerActivationThreshold(threshold)`, and - `parser.SetAllocTrackerMaximumAmplification(max_factor)`. (cherry picked from commit f04bea4)
…on API (python#139366) Fix some typos left in f04bea4, and simplify some internal functions to ease maintenance of future mitigation APIs. (cherry picked from commit 68a1778)
@hartworkhartwork requested a review from picnixzOctober 3, 2025 15:29
Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the backport!

#include<stdbool.h>
#include"structmember.h"// PyMemberDef
#include"frameobject.h"
#include<stddef.h>// offsetof()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need offsetof?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@picnixz we do!

For one, because it is used in:

staticPyMemberDefxmlparse_members[] ={{"intern", T_OBJECT, offsetof(xmlparseobject, intern), READONLY, NULL},{NULL} };

For two, because it is added in de690f1, the source of the backport.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum. Why did add it? I think it was because my IDE complained at some point. Ok then.

Copy link
ContributorAuthor

@hartworkhartworkOct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@picnixz I need to correct me earlier statement: I got the commit wrong, it's actually f04bea4 and you did not add that import there, it was already present in main and 3.14 and 3.13. So the question now seems to be whether we want to add or not add that import in the backports targetting 3.12, 3.11 and 3.10. It's the correct header but then some other header must be already pulling it in or the code would not compile already prior to these backports. I'm good with dropping or keeping that new include — what would you prefer?

Copy link
Member

@picnixzpicnixzOct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to reduce the diff as much as possible. If it works without the include then let's not add it. I think it would make better sense to remove it from 3.13 and 3.14 actually but I don't know if other files actually have an explicit or an implicit include policy...

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to reduce the diff as much as possible. If it works without the include then let's not add it.

@picnixz okay, let me try…

I think it would make better sense to remove it from 3.13 and 3.14 actually but I don't know if other files actually have an explicit or an implicit include policy...

That I would have trouble supporting. Please don't.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@picnixz PS: I found…

Doc/c-api/structures.rst: (You may need to ``#include <stddef.h>`` for :c:func:`!offsetof`.) 

…and also…

Include/structmember.h:#include <stddef.h> /* For offsetof (not always provided by Python.h) */ 

in 3.12 code and that all of 3.10, 3.11, 3.12 have pyexpat.c include structmember.h but 3.13, 3.14, main do not and so these seem to rightfully contain a dedicated #include <stddef.h> for offsetof.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@picnixz okay, let me try…

Update: dropped from all of 3.10, 3.11, 3.12 backports by now.

@picnixz
Copy link
Member

To have a good synchronization, we'll also delay 3.10 to 3.13 backports for their next release cycle (see #139359 (comment)).

@picnixzpicnixz self-assigned this Oct 7, 2025
@ambv
Copy link
Contributor

ambv commented Oct 8, 2025

I set DO-NOT-MERGE to avoid confusion. Unset that when you think we should be releasing this.

@hartwork
Copy link
ContributorAuthor

@pablogsal I believe this is ready to be merged. Do you have a minute?

@picnixz
Copy link
Member

This was this PR I wanted to check manually because we didn't have _Py_ID() access in 3.10

@hartwork
Copy link
ContributorAuthor

@pablogsal do you have a minute for this? 🙏

@pablogsalpablogsal merged commit 1173f80 into python:3.10Nov 25, 2025
15 checks passed
@pablogsal
Copy link
Member

Thanks a lot for the backport and thanks for the patience :)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@hartwork@picnixz@ambv@pablogsal