-
Notifications
You must be signed in to change notification settings - Fork 19
The host's own spelling of an argument: launcher argv, -W/-X option values, and os.stat's descriptor and dir_fd forms #1066
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6be5c06
launcher: carry a command-line argument in the host's own spelling
youknowone c6aeb72
posix: give os.stat the descriptor and dir_fd argument forms
youknowone 0ed154d
posix: unwrap stat's arguments in signature order
youknowone 72e4cf0
posix: reject an unavailable dir_fd while unwrapping it
youknowone 218a5c4
launcher: carry -W, -X and PYTHONWARNINGS in the host's own spelling
youknowone File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
67 changes: 67 additions & 0 deletions
67
pyre/extra_tests/parity_tests/argv_undecodable_argument.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| """A command-line argument with no UTF-8 spelling reaches `sys.argv` as itself. | ||
|
|
||
| `targetpypystandalone.py:76-80` builds `sys.argv` with `space.newfilename`, | ||
| which is `fsdecode(newbytes(s))`, so an argument carrying a byte the filesystem | ||
| encoding cannot spell arrives as the surrogate escape that re-encodes to that | ||
| byte — not rejected, not replaced. `sys.orig_argv` carries the same value. | ||
|
|
||
| The argument is passed to a child, so the test needs no such name on disk: the | ||
| filesystem never sees it, only `execve` does. Windows has no byte argv at all | ||
| and takes the wide command line, so this shape does not exist there. | ||
| """ | ||
|
|
||
| import os | ||
| import subprocess | ||
| import sys | ||
|
|
||
| if sys.platform == "win32": | ||
| print("OK") | ||
| raise SystemExit | ||
|
|
||
| UNDECODABLE = b"pyre_undecodable_\xff" | ||
| ESCAPED = os.fsdecode(UNDECODABLE) | ||
|
|
||
| # The escape is what the filesystem decode produces, and it round-trips. | ||
| assert ESCAPED.endswith("\udcff"), ascii(ESCAPED) | ||
| assert os.fsencode(ESCAPED) == UNDECODABLE, ascii(ESCAPED) | ||
|
|
||
| CHILD = r""" | ||
| import os, sys | ||
| assert sys.argv[1:] == [os.fsdecode(%r), "plain"], ascii(sys.argv) | ||
| assert os.fsencode(sys.argv[1]) == %r, ascii(sys.argv[1]) | ||
| # `orig_argv` is the launcher's own line, so the argument appears there too, | ||
| # with the same escaping. | ||
| assert sys.argv[1] in sys.orig_argv, ascii(sys.orig_argv) | ||
| assert sys.orig_argv[-2:] == sys.argv[1:], ascii(sys.orig_argv) | ||
| print("child ok") | ||
| """ % (UNDECODABLE, UNDECODABLE) | ||
|
|
||
| result = subprocess.run( | ||
| [sys.executable, "-c", CHILD, ESCAPED, "plain"], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) | ||
| assert result.returncode == 0, (result.returncode, result.stderr) | ||
| assert result.stdout == b"child ok\n", result.stdout | ||
|
|
||
| # A script run the same way answers with the argument in argv[1], and the | ||
| # script's own path stays argv[0]. | ||
| import tempfile | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmp: | ||
| script = os.path.join(tmp, "show_argv.py") | ||
| with open(script, "w") as f: | ||
| f.write( | ||
| "import os, sys\n" | ||
| "print(os.fsencode(sys.argv[0]) == os.fsencode(%r))\n" % script | ||
| + "print(ascii(sys.argv[1]))\n" | ||
| ) | ||
| result = subprocess.run( | ||
| [sys.executable, script, ESCAPED], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) | ||
| assert result.returncode == 0, (result.returncode, result.stderr) | ||
| assert result.stdout == b"True\n" + ascii(ESCAPED).encode() + b"\n", result.stdout | ||
|
|
||
| print("OK") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| """`-W`, `-X` and PYTHONWARNINGS carry a value with no UTF-8 spelling. | ||
|
|
||
| These are free text, not identifiers: `app_main.py:785-786` splits an `-X` | ||
| value on the first `=` and puts both halves into `sys._xoptions` verbatim, and | ||
| `:892-906` appends the `-W` values and the PYTHONWARNINGS pieces to | ||
| `sys.warnoptions` verbatim. None of them is required to be spellable in UTF-8, | ||
| so a byte the filesystem encoding cannot spell arrives as the surrogate escape | ||
| that re-encodes to that byte — in the `_xoptions` key as much as in its value. | ||
|
|
||
| An option value never reaches the filesystem, so like | ||
| `argv_undecodable_argument.py` this needs no such name on disk and passes the | ||
| value to a child instead. Windows takes a wide command line and has no byte | ||
| argv, so this shape does not exist there. | ||
| """ | ||
|
|
||
| import os | ||
| import subprocess | ||
| import sys | ||
|
|
||
| if sys.platform == "win32": | ||
| print("OK") | ||
| raise SystemExit | ||
|
|
||
| ESC = os.fsdecode(b"\xff") | ||
| assert ESC == "\udcff", ascii(ESC) | ||
|
|
||
|
|
||
| def child(*args, env=None): | ||
| result = subprocess.run( | ||
| [sys.executable, *args], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| env=env, | ||
| ) | ||
| assert result.returncode == 0, (result.returncode, result.stderr) | ||
| return result.stdout.decode() | ||
|
|
||
|
|
||
| # -W keeps the value it was given. The warnings module rejects it as a filter | ||
| # later — it is not a valid action — but that is a separate stage, and the | ||
| # option list records what the command line said. | ||
| out = child("-W", "ignore" + ESC, "-c", "import sys; print(ascii(sys.warnoptions))") | ||
| assert "'ignore\\udcff'" in out, out | ||
|
|
||
| # -X splits on the first `=`; the value half keeps the escape. | ||
| out = child("-X", "k=v" + ESC, "-c", "import sys; print(ascii(sys._xoptions))") | ||
| assert out.strip() == "{'k': 'v\\udcff'}", out | ||
|
|
||
| # ... and so does the key half, which is a dict key, not a name. | ||
| out = child("-X", "k" + ESC + "=v", "-c", "import sys; print(ascii(sys._xoptions))") | ||
| assert out.strip() == "{'k\\udcff': 'v'}", out | ||
|
|
||
| # A bare -X with no `=` is the key, and its value is True. | ||
| out = child("-X", "bare" + ESC, "-c", "import sys; print(ascii(sys._xoptions))") | ||
| assert out.strip() == "{'bare\\udcff': True}", out | ||
|
|
||
| # Only the first `=` splits, so a value may carry more of them. | ||
| out = child("-X", "k=a=b" + ESC, "-c", "import sys; print(ascii(sys._xoptions))") | ||
| assert out.strip() == "{'k': 'a=b\\udcff'}", out | ||
|
|
||
| # PYTHONWARNINGS is the same free text arriving through the environment, and it | ||
| # is comma-separated: one undecodable piece must not cost the whole variable. | ||
| env = dict(os.environ) | ||
| env["PYTHONWARNINGS"] = "ignore" + ESC + ",error" | ||
| out = child("-c", "import sys; print(ascii(sys.warnoptions))", env=env) | ||
| assert "'ignore\\udcff'" in out, out | ||
| assert "'error'" in out, out | ||
|
|
||
| print("OK") |
215 changes: 215 additions & 0 deletions
215
pyre/extra_tests/parity_tests/os_stat_file_descriptor.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,215 @@ | ||
| """`os.stat` takes an open file descriptor where `os.lstat` does not. | ||
|
|
||
| `interp_posix.py:611` declares stat's path as `path_or_fd(allow_fd=True)` and | ||
| `:659` declares lstat's as `allow_fd=False`, so the descriptor form belongs to | ||
| one of them only — and that difference is also what makes their type errors name | ||
| different allowed types. | ||
|
|
||
| `os.stat in os.supports_fd` is unconditionally true (`os.py:148`, "fstat always | ||
| works"), so this is the capability the set has always advertised. | ||
|
|
||
| `do_stat` (`interp_posix.py:634-644`) tests the descriptor before anything else: | ||
| holding one, neither `dir_fd` nor `follow_symlinks` has a path to apply to, and | ||
| both rejections come before the platform's `dir_fd` availability is consulted. | ||
| """ | ||
|
|
||
| import os | ||
| import tempfile | ||
| import warnings | ||
|
|
||
| assert os.stat in os.supports_fd, "os.stat has always been advertised as fd-capable" | ||
|
|
||
| tmp = tempfile.mkdtemp() | ||
| path = os.path.join(tmp, "f") | ||
| with open(path, "wb") as f: | ||
| f.write(b"0123456789") | ||
|
|
||
| fd = os.open(path, os.O_RDONLY) | ||
| try: | ||
| by_fd = os.stat(fd) | ||
| by_path = os.stat(path) | ||
| assert by_fd.st_size == 10, by_fd.st_size | ||
| # The same file either way: the descriptor form is `fstat`, not a re-open. | ||
| assert (by_fd.st_ino, by_fd.st_dev) == (by_path.st_ino, by_path.st_dev) | ||
| assert os.stat(fd) == os.fstat(fd) | ||
|
|
||
| # `True` is an `int`, so it names descriptor 1. Whether a bool used as a | ||
| # descriptor warns is a separate question; the value is what matters here. | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") | ||
| assert os.stat(True).st_dev == os.fstat(1).st_dev | ||
|
|
||
| # Neither other argument has anything to apply to. | ||
| try: | ||
| os.stat(fd, dir_fd=fd) | ||
| except ValueError as exc: | ||
| assert str(exc) == "stat: can't specify dir_fd without matching path", str(exc) | ||
| else: | ||
| raise AssertionError("stat accepted dir_fd with a descriptor") | ||
|
|
||
| try: | ||
| os.stat(fd, follow_symlinks=False) | ||
| except ValueError as exc: | ||
| assert str(exc) == "stat: cannot use fd and follow_symlinks together", str(exc) | ||
| else: | ||
| raise AssertionError("stat accepted follow_symlinks with a descriptor") | ||
|
|
||
| # lstat takes no descriptor, and says so with its own name and its own | ||
| # allowed-type list. | ||
| try: | ||
| os.lstat(fd) | ||
| except TypeError as exc: | ||
| assert str(exc) == "lstat: path should be string, bytes or os.PathLike, not int", str(exc) | ||
| else: | ||
| raise AssertionError("lstat accepted a descriptor") | ||
|
|
||
| # The widened list appears only where the descriptor is allowed. | ||
| try: | ||
| os.stat(1.5) | ||
| except TypeError as exc: | ||
| expected = "stat: path should be string, bytes, os.PathLike or integer, not float" | ||
| assert str(exc) == expected, str(exc) | ||
| else: | ||
| raise AssertionError("stat accepted a float") | ||
|
|
||
| try: | ||
| os.lstat(1.5) | ||
| except TypeError as exc: | ||
| expected = "lstat: path should be string, bytes or os.PathLike, not float" | ||
| assert str(exc) == expected, str(exc) | ||
| else: | ||
| raise AssertionError("lstat accepted a float") | ||
|
|
||
| # The arguments are unwrapped in signature order, so with more than one of | ||
| # them bad it is the leftmost that answers. Each can also run user code — | ||
| # `__fspath__`, `__index__`, `__bool__` — so the order is observable even | ||
| # when nothing raises. | ||
| try: | ||
| os.stat(1.5, dir_fd=1.5) | ||
| except TypeError as exc: | ||
| expected = "stat: path should be string, bytes, os.PathLike or integer, not float" | ||
| assert str(exc) == expected, str(exc) | ||
| else: | ||
| raise AssertionError("stat accepted a float path") | ||
|
|
||
| order = [] | ||
|
|
||
| class Spy: | ||
| def __fspath__(self): | ||
| order.append("path") | ||
| return tmp | ||
|
|
||
| class Truthy: | ||
| def __bool__(self): | ||
| order.append("follow_symlinks") | ||
| return True | ||
|
|
||
| try: | ||
| os.stat(Spy(), dir_fd=1.5, follow_symlinks=Truthy()) | ||
| except TypeError as exc: | ||
| assert str(exc) == "argument should be integer or None, not float", str(exc) | ||
| else: | ||
| raise AssertionError("stat accepted a float dir_fd") | ||
| # `path` was resolved, `dir_fd` then rejected, `follow_symlinks` never read. | ||
| assert order == ["path"], order | ||
|
|
||
| # The descriptor probe is `__index__`, and an object carrying both it and | ||
| # `__fspath__` is taken as a descriptor — so an `__index__` that raises | ||
| # reports its own exception instead of falling through to the path. | ||
| class BadIndex: | ||
| def __index__(self): | ||
| raise RuntimeError("boom") | ||
|
|
||
| def __fspath__(self): | ||
| return path | ||
|
|
||
| try: | ||
| os.stat(BadIndex()) | ||
| except RuntimeError as exc: | ||
| assert str(exc) == "boom", str(exc) | ||
| else: | ||
| raise AssertionError("stat fell through a raising __index__ to __fspath__") | ||
|
|
||
| # lstat takes no descriptor, so it never probes __index__ at all. | ||
| assert os.lstat(BadIndex()).st_size == 10 | ||
|
|
||
| # A descriptor no call can serve reports the descriptor, not the type: -1 | ||
| # is an OSError either way. Which errno it carries is a property of the | ||
| # libc path taken and differs between the two entry points even upstream | ||
| # (EFAULT from `stat`, EBADF from `fstat`), so only the class is pinned. | ||
| try: | ||
| os.stat(-1) | ||
| except OSError: | ||
| pass | ||
| else: | ||
| raise AssertionError("os.stat(-1) did not fail") | ||
| finally: | ||
| os.close(fd) | ||
|
|
||
| # `dir_fd` is a separate capability, reported honestly: a platform that does | ||
| # not honour it says so, and a platform that does resolves a relative name | ||
| # against the descriptor. `os.py:120-121` reads HAVE_FSTATAT for `stat` and | ||
| # HAVE_LSTAT for `lstat`, so the two are advertised independently. | ||
| if os.stat in os.supports_dir_fd: | ||
| assert os.lstat in os.supports_dir_fd, "lstat takes dir_fd wherever stat does" | ||
| link = os.path.join(tmp, "link") | ||
| os.symlink("f", link) | ||
| dfd = os.open(tmp, os.O_RDONLY) | ||
| try: | ||
| # `fstatat` resolves the name against the descriptor, and reaches the | ||
| # same file the path form does. | ||
| by_dir_fd = os.stat("f", dir_fd=dfd) | ||
| assert by_dir_fd.st_size == 10, by_dir_fd.st_size | ||
| assert (by_dir_fd.st_ino, by_dir_fd.st_dev) == (by_path.st_ino, by_path.st_dev) | ||
|
|
||
| # An absolute name ignores the descriptor entirely. | ||
| assert os.stat(path, dir_fd=dfd).st_ino == by_path.st_ino | ||
|
|
||
| # AT_SYMLINK_NOFOLLOW is what carries follow_symlinks=False, so the | ||
| # two spellings of "do not follow" agree. | ||
| nofollow = os.stat("link", dir_fd=dfd, follow_symlinks=False) | ||
| assert nofollow.st_ino == os.lstat("link", dir_fd=dfd).st_ino | ||
| assert nofollow.st_ino == os.lstat(link).st_ino | ||
| assert nofollow.st_ino != by_path.st_ino, "lstat followed the symlink" | ||
| assert os.stat("link", dir_fd=dfd).st_ino == by_path.st_ino | ||
|
|
||
| # A missing name reports the name, not the descriptor. | ||
| try: | ||
| os.stat("absent", dir_fd=dfd) | ||
| except FileNotFoundError as exc: | ||
| assert exc.filename == "absent", exc.filename | ||
| else: | ||
| raise AssertionError("stat found a name that does not exist") | ||
|
|
||
| # A descriptor that is not a directory cannot resolve a relative name. | ||
| plain = os.open(path, os.O_RDONLY) | ||
| try: | ||
| os.stat("f", dir_fd=plain) | ||
| except NotADirectoryError: | ||
| pass | ||
| else: | ||
| raise AssertionError("stat resolved a name against a plain file") | ||
| finally: | ||
| os.close(plain) | ||
|
|
||
| # `_unwrap_dirfd` types the argument before it reaches the syscall. | ||
| try: | ||
| os.stat("f", dir_fd=1.5) | ||
| except TypeError as exc: | ||
| assert str(exc) == "argument should be integer or None, not float", str(exc) | ||
| else: | ||
| raise AssertionError("stat accepted a float dir_fd") | ||
|
|
||
| # A descriptor no call can serve is an OSError. Which errno it carries | ||
| # depends on where the rejection happens — the sentinel check or the | ||
| # syscall — so only the class is pinned. | ||
| try: | ||
| os.stat("f", dir_fd=-1) | ||
| except OSError: | ||
| pass | ||
| else: | ||
| raise AssertionError("stat accepted dir_fd=-1") | ||
| finally: | ||
| os.close(dfd) | ||
|
|
||
| print("OK") | ||
Oops, something went wrong.
Oops, something went wrong.
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.
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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a negative-descriptor case to this suite.
The suite covers
-1through the descriptor path, but no case covers a descriptor below-1. The implementation guards-1only, in bothpath_or_fd_wandfstat_fd, soos.stat(-2)andos.fstat(-2)currently reachFile::from_raw_fdwith a negative value. A parity case that records the CPython 3.14 result foros.stat(-2)andos.fstat(-2)would pin the intended behavior alongside the implementation fix.I can generate that test case if you want it.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 120-120: Missing return type annotation for special method
__index__Add return type annotation:
int(ANN204)
[warning] 123-123: Missing return type annotation for special method
__fspath__(ANN204)
[warning] 129-129: Found assertion on exception
excinexceptblock, usepytest.raises()instead(PT017)
[warning] 129-129: Found assertion on exception
excinexceptblock, usepytest.raises()instead(PT017)
[warning] 131-131: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents