GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-94526: getpath_dirname() no longer encodes the path by vstinner · Pull Request #97645 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Sep 29, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Fix the Python path configuration used to initialized sys.path at
Python startup. getpath_basename() and getpath_dirname() functions no
longer encode the path to UTF-8/strict to avoid encoding errors if it
contains surrogate characters (created by decoding a bytes path with
the surrogateescape error handler).

The functions now use PyUnicode_FindChar() and PyUnicode_Substring()
on the Unicode path, rather than strrchr() on the encoded bytes
string.

  • Issue: Exception in ucs2lib_utf8_encoder in _bootstrap_python #94526

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner vstinner added the needs backport to 3.11 only security fixes label Sep 29, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Sep 29, 2022
@vstinner vstinner mentioned this pull request Sep 29, 2022
Exception in ucs2lib_utf8_encoder in _bootstrap_python #94526
Closed
@vstinner

vstinner commented Sep 29, 2022

Copy link
Copy Markdown
Member Author

Fix building Python in a non-ASCII path

Well. In fact, the issue is broader: no only _bootstrap_python is affected, any python program is affected since the Modules/getpath.c code is used by all Python executables.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Sep 29, 2022

Copy link
Copy Markdown
Member Author

I rebased and updated the PR to clarify that this issue affects the Python path configuration (sys.path creation).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Sep 29, 2022

Copy link
Copy Markdown
Member Author

Sadly, Modules/getpath.c is not a regular extension module, it cannot be loaded in test_getpath to easily write unit tests.

There are getpath_methods which are injected inside a namespace (dict) by funcs_to_dict() function.

It may be interesting to convert it to a regular extension module (_getpath?).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Sep 29, 2022
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-09-29-15-19-29.gh-issue-94526.wq5m6T.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/getpath.c
const char *path;
if (!PyArg_ParseTuple(args, "s", &path)) {
PyObject *path;
if (!PyArg_ParseTuple(args, "U", &path)) {

@serhiy-storchaka serhiy-storchaka Sep 29, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

BTW, I would use METH_O and PyArg_Parse() in these functions, but this is another issue.

Why cannot they be implemented in Python?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 29, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

BTW, I would use METH_O and PyArg_Parse() in these functions, but this is another issue.

I tried to minimize the changes.

Why cannot they be implemented in Python?

Ask @zooba who designed this. Maybe it can be changed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@zooba zooba Sep 29, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Perf, mostly. These trivial ones probably could be, but don't fall into the trap of trying to port the full ntpath/posixpath implementations into getpath - we don't have a lot of the functionality needed to handle those at this stage (e.g. no codecs, no os module).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Sep 29, 2022
@vstinner
gh-94526: getpath_dirname() no longer encodes the path
ac8255b 
Fix the Python path configuration used to initialized sys.path at
Python startup. Paths are no longer encoded to UTF-8/strict to avoid
encoding errors if it contains surrogate characters (bytes paths are
decoded with the surrogateescape error handler).

getpath_basename() and getpath_dirname() functions no longer encode
the path to UTF-8/strict, but work directly on Unicode strings. These
functions now use PyUnicode_FindChar() and PyUnicode_Substring() on
the Unicode path, rather than strrchr() on the encoded bytes string.
@vstinner

vstinner commented Sep 29, 2022

Copy link
Copy Markdown
Member Author

@serhiy-storchaka:

The NEWS entry is meant to be read by a common Python user, not a core dev. getpath_basename and getpath_dirname are not Python function. Could you rewrite this?

I rephrased the NEWS entry to omit function names. Is it better? I only named functions in the commit message.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Sep 29, 2022
View reviewed changes
@kumaraditya303

kumaraditya303 commented Sep 29, 2022

Copy link
Copy Markdown
Contributor

Although this fixes the issue, this is a bit fragile since it can break if any other function were to use utf8 handler for encoding.

This PR avoids the case, can you add a comment that utf8 should be avoided here?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit 9f2f1dd into python:main Sep 30, 2022
@miss-islington

miss-islington commented Sep 30, 2022

Copy link
Copy Markdown
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 30, 2022
@miss-islington

miss-islington commented Sep 30, 2022

Copy link
Copy Markdown
Contributor

Sorry @vstinner, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 9f2f1dd131b912e224cd0269adde8879799686c4 3.11
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner deleted the getpath_unicode branch September 30, 2022 12:58
@miss-islington miss-islington assigned vstinner Sep 30, 2022
@vstinner

vstinner commented Sep 30, 2022

Copy link
Copy Markdown
Member Author

Although this fixes the issue, this is a bit fragile since it can break if any other function were to use utf8 handler for encoding.

Yes, a regression can be introduced again tomorrow. Well, we can fix it in this case :-)

This PR avoids the case, can you add a comment that utf8 should be avoided here?

I'm not sure about the intent of a comment explaining that UTF-8 should not be used, since the modified functions now use Unicode (no encode/decode).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Sep 30, 2022
@miss-islington

miss-islington commented Sep 30, 2022

Copy link
Copy Markdown
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Sep 30, 2022

Copy link
Copy Markdown

GH-97677 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 30, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 30, 2022
@vstinner @miss-islington
pythongh-94526: getpath_dirname() no longer encodes the path (pythonG…
0aa7e74 
…H-97645)

Fix the Python path configuration used to initialized sys.path at
Python startup. Paths are no longer encoded to UTF-8/strict to avoid
encoding errors if it contains surrogate characters (bytes paths are
decoded with the surrogateescape error handler).

getpath_basename() and getpath_dirname() functions no longer encode
the path to UTF-8/strict, but work directly on Unicode strings. These
functions now use PyUnicode_FindChar() and PyUnicode_Substring() on
the Unicode path, rather than strrchr() on the encoded bytes string.
(cherry picked from commit 9f2f1dd)

Co-authored-by: Victor Stinner <[email protected]>
@kumaraditya303 kumaraditya303 mentioned this pull request Sep 30, 2022
GH-94526: Force utf8 encoding in _bootstrap_python #96889
Closed
miss-islington added a commit that referenced this pull request Sep 30, 2022
@miss-islington @vstinner
gh-94526: getpath_dirname() no longer encodes the path (GH-97645)
6537bc9 
Fix the Python path configuration used to initialized sys.path at
Python startup. Paths are no longer encoded to UTF-8/strict to avoid
encoding errors if it contains surrogate characters (bytes paths are
decoded with the surrogateescape error handler).

getpath_basename() and getpath_dirname() functions no longer encode
the path to UTF-8/strict, but work directly on Unicode strings. These
functions now use PyUnicode_FindChar() and PyUnicode_Substring() on
the Unicode path, rather than strrchr() on the encoded bytes string.
(cherry picked from commit 9f2f1dd)

Co-authored-by: Victor Stinner <[email protected]>
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 2, 2022
@vstinner @serhiy-storchaka
pythongh-94526: getpath_dirname() no longer encodes the path (python#…
cc1e8e0 
…97645)

Fix the Python path configuration used to initialized sys.path at
Python startup. Paths are no longer encoded to UTF-8/strict to avoid
encoding errors if it contains surrogate characters (bytes paths are
decoded with the surrogateescape error handler).

getpath_basename() and getpath_dirname() functions no longer encode
the path to UTF-8/strict, but work directly on Unicode strings. These
functions now use PyUnicode_FindChar() and PyUnicode_Substring() on
the Unicode path, rather than strrchr() on the encoded bytes string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zooba zooba zooba left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@vstinner vstinner

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@vstinner @kumaraditya303 @miss-islington @bedevere-bot @zooba @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.