Skip to content
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

bpo-46784: Add newly exported expat symbols to the namespace. #31397

Merged
merged 2 commits into from
Feb 18, 2022

Conversation

yilei
Copy link
Contributor

@yilei yilei commented Feb 17, 2022

The libexpat 2.4.1 upgrade from https://bugs.python.org/issue44394 introduced the following new exported symbols:

  • testingAccountingGetCountBytesDirect
  • testingAccountingGetCountBytesIndirect
  • unsignedCharToPrintable
  • XML_SetBillionLaughsAttackProtectionActivationThreshold
  • XML_SetBillionLaughsAttackProtectionMaximumAmplification

We need to adjust Modules/expat/pyexpatns.h

(The newer libexpat upgrade https://bugs.python.org/issue46400 has no new symbols).

https://bugs.python.org/issue46784

Automerge-Triggered-By: GH:gpshead

@yilei
Copy link
Contributor Author

yilei commented Feb 18, 2022

Should I create a news entry in the "Core and Builtins" category?

@gpshead gpshead added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Feb 18, 2022
@gpshead
Copy link
Member

gpshead commented Feb 18, 2022

Should I create a news entry in the "Core and Builtins" category?

I don't remember the categories, extension modules or library would be my first choices as this only impacts the xml modules which should be dynamically loaded in most people's builds. if those don't exist core and builtins is fine. the main thing is having a news entry. :)

@gpshead gpshead self-assigned this Feb 18, 2022
@gpshead gpshead self-requested a review February 18, 2022 01:30
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@hartwork

I'm not sure about the case of dynamically using an older version.
I want to ask for advice.

@gpshead
Copy link
Member

gpshead commented Feb 18, 2022

This PR is just continuing what CPython pyexpatns.h already does to avoid symbol conflicts between its own embedded C expat and anyone else linking in their own expat.

It comes up in embedding contexts where CPython is linked into a C/C++ application that also uses upstream expat itself. It'd presumably also be possible when a C/C++ extension module uses expat itself.

Dynamically, without this, code either uses the first version loaded or there's a duplicate symbol error upon loading the extension module using the second version. Which problem, if any, probably depends on exactly how things are linked.

@corona10
Copy link
Member

@gpshead Thank you for explain :)

@hartwork
Copy link
Contributor

The libexpat 2.4.1 upgrade from https://bugs.python.org/issue44394 introduced the following new exported symbols:

  • testingAccountingGetCountBytesDirect

  • testingAccountingGetCountBytesIndirect

  • unsignedCharToPrintable

I would like to note that these three^^ are not considered public API and that Expat's own two build systems (CMake and GNU Autotools) tell the linker which symbols are public and hide everything else. Maybe you'll decide that doesn't matter, but I wanted to be sure it's clear these are not public symbols of Expat.

  • XML_SetBillionLaughsAttackProtectionActivationThreshold

  • XML_SetBillionLaughsAttackProtectionMaximumAmplification

I still hope that someone can make those two^^ accessible (with additional glue code) to the user on pyexpat level in CPython.

@gpshead
Copy link
Member

gpshead commented Feb 18, 2022

  • XML_SetBillionLaughsAttackProtectionActivationThreshold
  • XML_SetBillionLaughsAttackProtectionMaximumAmplification

I still hope that someone can make those two^^ accessible

I filed https://bugs.python.org/issue46793 to at least track that.

@gpshead
Copy link
Member

gpshead commented Feb 18, 2022

regarding the symbol exports, as we're renaming them to not conflict I doubt it matters much - though it is effectively dead code in the shared library. Avoiding export requires modifying how the module gets linked, probably via flags in setup.py and possibly the PCbuild windows stuff. It would be nice, but if we were doing that we could've done renames via the linker or avoided exporting things at all? Though we might have _elementtree depend on getting them from pyexpat? I'm not going to let untangling our embedded expat copy mess hold this up. :)

@hartwork
Copy link
Contributor

it is effectively dead code in the shared library.

@gpshead not unsignedCharToPrintable but the other two, yes, interesting point.

I'm not going to let untangling our embedded expat copy mess hold this up. :)

That sounds healthy 👍

@gpshead gpshead added 🤖 automerge type-bug An unexpected behavior, bug, or error labels Feb 18, 2022
@yilei
Copy link
Contributor Author

yilei commented Feb 18, 2022

News entry added.

@miss-islington
Copy link
Contributor

@yilei: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 6312c10 into python:main Feb 18, 2022
@miss-islington
Copy link
Contributor

Thanks @yilei for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-31417 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Feb 18, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 18, 2022
…GH-31397)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
@miss-islington
Copy link
Contributor

Thanks @yilei for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @yilei for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-31418 is a backport of this pull request to the 3.7 branch.

@bedevere-bot
Copy link

GH-31419 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 18, 2022
…GH-31397)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
@gpshead gpshead added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes labels Feb 18, 2022
@miss-islington
Copy link
Contributor

Thanks @yilei for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington added a commit that referenced this pull request Feb 18, 2022
The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
@gpshead gpshead added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes labels Feb 18, 2022
@miss-islington
Copy link
Contributor

Thanks @yilei for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 18, 2022
…GH-31397)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
gpshead pushed a commit to gpshead/cpython that referenced this pull request Feb 18, 2022
…ythonGH-31397)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 18, 2022
@bedevere-bot
Copy link

GH-31420 is a backport of this pull request to the 3.10 branch.

gpshead added a commit that referenced this pull request Feb 19, 2022
…H-31397) (GH-31420)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
ned-deily pushed a commit that referenced this pull request Feb 21, 2022
…) (GH-31418)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
ambv pushed a commit to miss-islington/cpython that referenced this pull request Mar 8, 2022
…GH-31397)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
ambv pushed a commit that referenced this pull request Mar 8, 2022
…) (GH-31419)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
…GH-31397)

The libexpat 2.4.1 upgrade from  introduced the following new exported symbols:

* `testingAccountingGetCountBytesDirect`
* `testingAccountingGetCountBytesIndirect`
* `unsignedCharToPrintable`
* `XML_SetBillionLaughsAttackProtectionActivationThreshold`
* `XML_SetBillionLaughsAttackProtectionMaximumAmplification`

We need to adjust [Modules/expat/pyexpatns.h](https://github.com/python/cpython/blob/master/Modules/expat/pyexpatns.h)

(The newer libexpat upgrade  has no new symbols).

Automerge-Triggered-By: GH:gpshead
(cherry picked from commit 6312c10)

Co-authored-by: Yilei "Dolee" Yang <yileiyang@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants