Skip to content

support: use the 'data' tar extraction filter in install-share.py - #1247

Open
takano32 wants to merge 2 commits into
avast:masterfrom
takano32:fix/install-share-tar-filter
Open

takano32 wants to merge 2 commits into
avast:masterfrom
takano32:fix/install-share-tar-filter

Conversation

@takano32

Copy link
Copy Markdown
Contributor

tarfile.extractall() without an explicit filter emits a DeprecationWarning on Python 3.12/3.13 during make install, and Python 3.14 already defaults to the 'data' filter (PEP 706). Request the 'data' filter explicitly where available, using the feature check documented in the tarfile docs (hasattr(tarfile, 'data_filter'), present also in the security backports to older 3.x releases); Python versions without extraction filters keep the current behavior unchanged.

The support package extracts cleanly under the 'data' filter — it is the behavior already applied by default on Python 3.14 installs today (verified with an end-to-end make install on Python 3.14).

🤖 Generated with Claude Code

https://claude.ai/code/session_014nNMVbAuffaPPQuYdLZgD1

tarfile.extractall() without an explicit filter emits a
DeprecationWarning on Python 3.12/3.13 during 'make install', and
Python 3.14 already defaults to the 'data' filter (PEP 706). Request
the 'data' filter explicitly where available (the documented
hasattr(tarfile, 'data_filter') feature check), keeping older Python
versions working unchanged.

The support package extracts cleanly under the 'data' filter (it is
the default-applied behavior on Python 3.14 installs today).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014nNMVbAuffaPPQuYdLZgD1

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee0d353112

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread support/install-share.py
try:
tar.extractall(support_dir)
if hasattr(tarfile, 'data_filter'):
tar.extractall(support_dir, filter='data')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Clean up data-filter extraction failures

When the data filter rejects a member (for example, a custom support package with a path outside the destination), tarfile raises FilterError, not ExtractError, so this new path bypasses the cleanup block. If version.txt was extracted before the rejected member, the next make install run will hit the early “Up-to-date” check and succeed with only a partial support directory left behind.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — fixed in 43c1fe5.

FilterError is a direct subclass of TarError (a sibling of ExtractError, not a subclass), so the handler indeed let filter rejections bypass the cleanup path. The except now catches tarfile.TarError, which covers ExtractError, FilterError where the filter feature exists, and ReadError for truncated archives — any extraction failure now takes the cleanup(support_dir) + exit(1) path.

Verified with a crafted archive containing a path-escaping member: previously OutsideDestinationError escaped the ExtractError handler and left the already-extracted members behind; with the fix it is caught and the support dir is cleaned up.

Worth noting this failure mode is not introduced by this PR: on Python 3.14 extractall() already defaults to the 'data' filter, so current master has the same uncleaned-partial-extraction path — broadening the except fixes it for both.

As pointed out in review, extraction-filter rejections raise
tarfile.FilterError, which is a sibling of ExtractError under
TarError, so the previous handler let them bypass the
cleanup(support_dir) path and leave a partial support directory
behind. Catch tarfile.TarError, which covers ExtractError,
FilterError (where the filter feature exists), and ReadError for
truncated archives. On Python 3.14 extractall() already defaults to
the 'data' filter, so this uncaught path exists on master today as
well.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014nNMVbAuffaPPQuYdLZgD1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant