Repository navigation
bpo-32381: .pyc files with non-ASCII paths cannot be reopened on Windows - #14699
ZackerySpytz wants to merge 1 commit into
Conversation
Sorry, something went wrong.
| return NULL; | ||
| if (make_non_inheritable(fileno(f)) < 0) { | ||
| fclose(f); | ||
| PyObject *path = PyUnicode_DecodeFSDefault(pathname); |
There was a problem hiding this comment.
The change should be conditionally defined just for Windows. In the Unix world a path is just a sequence of bytes, with only ASCII slash and null reserved.
Sorry, something went wrong.
There was a problem hiding this comment.
Agreed with @eryksun. Performing the change across all OS's wouldn't be necessary, since this is a windows specific issue.
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you, @eryksun, for the comment. One issue is the fact that _Py_fopen_obj() calls set_inheritable() with raise == 1, but _Py_fopen() currently uses make_non_inheritable(), which calls set_inheritable() with raise == 0. I think, for non-Windows, the make_non_inheritable() call in _Py_fopen() should be replaced with set_inheritable(fileno(f), 0, 1, NULL).
Sorry, something went wrong.
There was a problem hiding this comment.
It's a problem that _Py_fopen doesn't require the GIL to be held, but _Py_fopen_obj requires it. It may be better to modify PyRun_SimpleFileExFlags instead of tinkering with _Py_fopen.
(Another issue is with pymain_run_startup, which gets the ANSI encoded PYTHONSTARTUP filename via _Py_GetEnv and passes it to _Py_fopen. This is a problem for users with arbitrary Unicode usernames if the startup file is in their profile directory. In Windows we need to pass the filename from the native wide-character environment to _Py_wfopen.)
Sorry, something went wrong.
| self.assertEqual(b"", out) | ||
| self.assertEqual(b"", err) | ||
|
|
||
| def test_issue32381(self): |
There was a problem hiding this comment.
Instead of test_issue32381, I would recommend renaming the method to something more descriptive of the issue itself, such as test_windows_non_ascii_path.
| def test_issue32381(self): | |
| # tests bpo-32381 | |
| def test_windows_non_ascii_path(self): |
Sorry, something went wrong.
| return NULL; | ||
| if (make_non_inheritable(fileno(f)) < 0) { | ||
| fclose(f); | ||
| PyObject *path = PyUnicode_DecodeFSDefault(pathname); |
There was a problem hiding this comment.
Agreed with @eryksun. Performing the change across all OS's wouldn't be necessary, since this is a windows specific issue.
Sorry, something went wrong.
|
I'm not comfortable with changing _Py_fopen(), so I wrote PR #23642 to use _Py_fopen_obj() in pythonrun.c. Would you mind to review it? |
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
I dislike the idea of calling PyUnicode_DecodeFSDefault() on any platform. On Unix, fopen() is the native API. It should only be done on Windows.
I fixed https://bugs.python.org/issue32381 differently. In general, I would suggest avoiding _Py_fopen() and prefer the variant accept a wide string (wchar_t*) or a Python object (PyObject*).
Should we close this PR?
pymain_run_startup() calls _Py_fopen() on Unix and _Py_wfopen() on Windows.
PyErr_ProgramText() calls _Py_fopen(), but Python no longer calls PyErr_ProgramText() anymore.
Sorry, something went wrong.
|
I removed _Py_fopen() in PR #23711, so this fix is not longer needed. Moreover, I fixed https://bugs.python.org/issue32381 encoding issue differently: commit b6d98c1. |
Sorry, something went wrong.
https://bugs.python.org/issue32381