Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-115399: Upgrade bundled libexpat to 2.6.0#115431

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Yhg1s merged 2 commits intopython:mainfromsethmlarson:libexpat-2.6.0
Feb 14, 2024

Conversation

@sethmlarson
Copy link
Contributor

@sethmlarsonsethmlarson commentedFeb 13, 2024
edited
Loading

Closes#115399

  • Upgrades libexpat to 2.6.0
  • AddsXML_GE definition toLIBEXPAT_CFLAGS since 2.6.0 requires us to choose one, I set it to1 to keep general entities support. Our test suite didn't fail either way with this setting but I'm assuming we want to keep support?

cc@hartwork

hartwork reacted with thumbs up emoji
configure.ac Outdated
LIBEXPAT_INTERNAL=
],[
LIBEXPAT_CFLAGS="-I\$(srcdir)/Modules/expat"
LIBEXPAT_CFLAGS="-I\$(srcdir)/Modules/expat -DXML_GE=1"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's that?

Copy link
ContributorAuthor

@sethmlarsonsethmlarsonFeb 13, 2024
edited
Loading

Choose a reason for hiding this comment

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

We have to make a choice, error message from the Windows builds that are now failing cuz I need to update them as well:

XML_GE (for general entities) must be defined, non-empty, either 1 or 0 (0 to disable, 1 to enable; 1 is a common default

hartwork reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably update the default value in line 3719 as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

what's that?

@ambv "GE" is short for general entities. The flag is documented in detail athttps://libexpat.github.io/doc/api/latest/#XML_GE .

@sethmlarson I would have expected (meaning considered likely) a new line#define XML_GE 1 to go intoCPython's ownexpat_config.h. Did you avoid that file on purpose? Its siblingXML_DTD seems to live there.

You should probably update the default value in line 3719 as well.

For anyone else wondering where (or what the line content is), that seems to be:

LIBEXPAT_CFLAGS=${LIBEXPAT_CFLAGS-""}

ambv and gpshead reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@hartwork Thank you for this, I don't know how but mygrep XML_DTD turned up no results. That's definitely where it should go.

hartwork reacted with thumbs up emoji
@hartwork
Copy link
Contributor

[..] I set it to1 to keep general entities support. Our test suite didn't fail either way with this setting but I'm assuming we want to keep support?

@sethmlarson good call, yes. On a sidenoteXML_DTD requiresXML_GE so it's the only choice here withXML_DTD active anyway. Good 👍

sethmlarson and gpshead reacted with heart emoji

@sethmlarson
Copy link
ContributorAuthor

Thanks for the guidance@hartwork! I rearranged the configuration to do as you said.

I also split the update itself into aseparate commit from the SBOM update (which removesexpat/expat_config.h from being a part of the expat project) so it's easier to backport to older branches. This PR can directly be backported to 3.12 but 3.11 and earlier will need onlya011b5f

hartwork reacted with thumbs up emoji

Comment on lines 67 to 71

/* Namespace external symbols to allow multiple libexpat version to
co-exist.*/
#include"pyexpatns.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

@sethmlarson could it be that this removal is accidental?

PS: Here's the Dockerfile that made me spot this, based on earlier version#98742 (review):

# Copyright (c) 2022-2024 Sebastian Pipping <sebastian@pipping.org># Licensed under the Apache License version 2.0FROM alpineRUN apk add --update \            diffutils \            git \            sed \        && \    git clone --depth 1 https://github.com/python/cpython cpython-main \        && \    ( cd cpython-main && git rev-parse HEAD ) \        && \    git clone --depth 1 --branch libexpat-2.6.0 https://github.com/sethmlarson/cpython cpython-pr \        && \    ( cd cpython-pr && git rev-parse HEAD ) \        && \    git config --global advice.detachedHead false \        && \    git clone --depth 1 --branch R_2_5_0 https://github.com/libexpat/libexpat libexpat_2_5_0 \        && \    git clone --depth 1 --branch R_2_6_0 https://github.com/libexpat/libexpat libexpat_2_6_0 \        && \    diff -r -u libexpat_2_5_0/expat/lib/ cpython-main/Modules/expat/ | tee 2-5-0.diff \        && \    diff -r -u libexpat_2_6_0/expat/lib/ cpython-pr/Modules/expat/ | tee 2-6-0.diff \        && \    sed -e'/^Only in /d' -e'/^\(+++\|---\) /d' -e'/^diff /d' -i 2-5-0.diff 2-6-0.diff \        && \    diff -u 2-5-0.diff 2-6-0.diff \        && \    echo'Diff is good.'

Copy link
Member

Choose a reason for hiding this comment

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

@hugovk It feels like now we have to automate this diff comparison. Do you have any good ideas on when to trigger the workflow smartly?

Copy link
Contributor

Choose a reason for hiding this comment

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

@corona10 did you mean me or@hugovk?

To automate this, you'd need to extract an old and a new version from the diff between the push HEAD and the base commit and so on. I'm not sure if Expat releases are frequent enough to build automation like that and keep it working.

Pull request#100539 is about something related but different, but maybe still of interest to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@corona10 PS: maybe a cheap version to cover the core need (unless misunderstood) would be to make CI assert that that file still includes filepyexpatns.h using grep? That would be orders of magnitude simpler 😃

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for catching this, I've updated my commit to not remove these lines.

hartwork reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

@hartwork
I meant@hugovk since he has good insight into automation workflow in CPython.
But in any case, the link you introduced looks cool to me, I think that it would be worth to take a look at :)

@sethmlarsonsethmlarson added the needs backport to 3.12only security fixes labelFeb 14, 2024
@sethmlarson
Copy link
ContributorAuthor

Backport to 3.11 and prior of this PR:#115468

3.12 will be handled automatically once this PR merges.

hartwork reacted with thumbs up emoji

@Yhg1sYhg1s merged commit4b2d178 intopython:mainFeb 14, 2024
@miss-islington-app
Copy link

Thanks@sethmlarson for the PR, and@Yhg1s for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestFeb 14, 2024
(cherry picked from commit4b2d178)Co-authored-by: Seth Michael Larson <seth@python.org>
@bedevere-app
Copy link

GH-115469 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelFeb 14, 2024
@sethmlarsonsethmlarson deleted the libexpat-2.6.0 branchFebruary 14, 2024 18:08
@gpsheadgpshead added the type-securityA security issue labelFeb 14, 2024
gpshead pushed a commit that referenced this pull requestFeb 14, 2024
)gh-115399: Upgrade bundled libexpat to 2.6.0 (GH-115431)(cherry picked from commit4b2d178)Co-authored-by: Seth Michael Larson <seth@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ambvambvambv approved these changes

@Yhg1sYhg1sYhg1s approved these changes

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aasland

@corona10corona10Awaiting requested review from corona10

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

+1 more reviewer

@hartworkhartworkhartwork approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type-securityA security issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Please upgrade bundled Expat to 2.6.0 (e.g. for the fix to CVE-2023-52425)

7 participants

@sethmlarson@hartwork@ambv@Yhg1s@corona10@erlend-aasland@gpshead

[8]ページ先頭

©2009-2025 Movatter.jp