Skip to content
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

Fix crash when passing a very long cmdline argument #11404

Merged
merged 2 commits into from Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/11394.bugfix.rst
@@ -0,0 +1 @@
Fixed crash when parsing long command line arguments that might be interpreted as files.
3 changes: 2 additions & 1 deletion src/_pytest/main.py
Expand Up @@ -36,6 +36,7 @@
from _pytest.pathlib import absolutepath
from _pytest.pathlib import bestrelpath
from _pytest.pathlib import fnmatch_ex
from _pytest.pathlib import safe_exists
from _pytest.pathlib import visit
from _pytest.reports import CollectReport
from _pytest.reports import TestReport
Expand Down Expand Up @@ -888,7 +889,7 @@ def resolve_collection_argument(
strpath = search_pypath(strpath)
fspath = invocation_path / strpath
fspath = absolutepath(fspath)
if not fspath.exists():
if not safe_exists(fspath):
msg = (
"module or package not found: {arg} (missing __init__.py?)"
if as_pypath
Expand Down
11 changes: 11 additions & 0 deletions src/_pytest/pathlib.py
@@ -1,5 +1,6 @@
import atexit
import contextlib
import errno
import fnmatch
import importlib.util
import itertools
Expand Down Expand Up @@ -773,3 +774,13 @@
# Forward from base to dest.
*reldest.parts,
)


def safe_exists(p: Path) -> bool:
"""Like Path.exists(), but account for input arguments that might be too long (#11394)."""
try:
return p.exists()
except OSError as e:

Check warning on line 783 in src/_pytest/pathlib.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/pathlib.py#L783

Added line #L783 was not covered by tests
if e.errno == errno.ENAMETOOLONG:
return False
raise

Check warning on line 786 in src/_pytest/pathlib.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/pathlib.py#L785-L786

Added lines #L785 - L786 were not covered by tests
31 changes: 31 additions & 0 deletions testing/test_main.py
Expand Up @@ -262,3 +262,34 @@ def test(fix):
"* 1 passed in *",
]
)


def test_very_long_cmdline_arg(pytester: Pytester) -> None:
Copy link
Member Author

@nicoddemus nicoddemus Sep 7, 2023

Choose a reason for hiding this comment

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

@iandonnelly I'm having trouble reproducing the original problem, this test is passing on all platforms.

Any hints?

Copy link
Member

Choose a reason for hiding this comment

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

its possibly related to available file-systems/path types (i believe the github runners always use unc/long paths)

would it break when using a path thats not supported on all platforms (i believe >64k)

Copy link
Member Author

@nicoddemus nicoddemus Sep 7, 2023

Choose a reason for hiding this comment

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

Hmm thanks for the hint, trying now with a very large path (>100k), let's see.

Copy link
Member Author

@nicoddemus nicoddemus Sep 7, 2023

Choose a reason for hiding this comment

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

Btw anybody on linux can test this please?

>>> import errno
>>> errno.ENAMETOOLONG
38

The above is on Windows; on the original report, the errno was 36 on Linux, want to confirm that's ENAMETOOLONG is indeed 36.

EDIT: nevermind, just confirmed it is indeed 36 on Linux.

Copy link
Member Author

@nicoddemus nicoddemus Sep 7, 2023

Choose a reason for hiding this comment

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

Hmm no good using >100k filenames, the test still passes.

I guess we can rely on patching then to ensure the behavior...

"""
Regression test for #11394.

Note: we could not manage to actually reproduce the error with this code, we suspect
GitHub runners are configured to support very long paths, however decided to leave
the test in place in case this ever regresses in the future.
"""
pytester.makeconftest(
"""
import pytest

def pytest_addoption(parser):
parser.addoption("--long-list", dest="long_list", action="store", default="all", help="List of things")

@pytest.fixture(scope="module")
def specified_feeds(request):
list_string = request.config.getoption("--long-list")
return list_string.split(',')
"""
)
pytester.makepyfile(
"""
def test_foo(specified_feeds):
assert len(specified_feeds) == 100_000
"""
)
result = pytester.runpytest("--long-list", ",".join(["helloworld"] * 100_000))
result.stdout.fnmatch_lines("* 1 passed *")
32 changes: 32 additions & 0 deletions testing/test_pathlib.py
@@ -1,3 +1,4 @@
import errno
import os.path
import pickle
import sys
Expand All @@ -24,6 +25,7 @@
from _pytest.pathlib import maybe_delete_a_numbered_dir
from _pytest.pathlib import module_name_from_path
from _pytest.pathlib import resolve_package_path
from _pytest.pathlib import safe_exists
from _pytest.pathlib import symlink_or_skip
from _pytest.pathlib import visit
from _pytest.tmpdir import TempPathFactory
Expand Down Expand Up @@ -660,3 +662,33 @@ def __init__(self) -> None:

mod = import_path(init, root=tmp_path, mode=ImportMode.importlib)
assert len(mod.instance.INSTANCES) == 1


def test_safe_exists(tmp_path: Path) -> None:
d = tmp_path.joinpath("some_dir")
d.mkdir()
assert safe_exists(d) is True

f = tmp_path.joinpath("some_file")
f.touch()
assert safe_exists(f) is True

# Use unittest.mock() as a context manager to have a very narrow
# patch lifetime.
p = tmp_path.joinpath("some long filename" * 100)
with unittest.mock.patch.object(
Path,
"exists",
autospec=True,
side_effect=OSError(errno.ENAMETOOLONG, "name too long"),
):
assert safe_exists(p) is False

with unittest.mock.patch.object(
Path,
"exists",
autospec=True,
side_effect=OSError(errno.EIO, "another kind of error"),
):
with pytest.raises(OSError):
_ = safe_exists(p)