only load a stdlib-named C extension from the standard library

[kali834x]

Type of Changes

	Type
✓	🐛 Bug fix

Description

_can_load_extension only receives the module name, and is_stdlib_module is a
plain name test, so any C extension whose name matches a stdlib module is
imported no matter where it came from. ImportlibFinder.find_module walks
sys.path in order and tries the extension suffixes before .py, and the
analysed project's directory comes first, so a colorsys.cpython-313-x86_64-linux-gnu.so
committed to a repository resolves ahead of the real module and
ast_from_module_name hands it to load_module_from_name, which runs its
module init. That happens with always_load_extensions off and an empty
extension-pkg-allow-list, so the opt-in that guards extension loading is
skipped entirely and linting a checkout executes code out of it. Pass the
resolved location down and require it to be inside STD_LIB_DIRS before the
stdlib name is trusted, excluding EXT_LIB_DIRS because site-packages sits
below the stdlib directory in a virtualenv. Extensions shipped with the
interpreter still load from lib-dynload, and an explicit allow-list entry
still wins.

[codspeed-hq]

Merging this PR will not alter performance

✅ 3 untouched benchmarks
⏩ 1 skipped benchmark¹

---

_{Comparing kali834x:stdlib-extension-location (c9124da) with main (94b7090)}

Footnotes

1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.65%. Comparing base (94b7090) to head (c9124da).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3193   +/-   ##
=======================================
  Coverage   93.65%   93.65%           
=======================================
  Files          93       93           
  Lines       11613    11618    +5     
=======================================
+ Hits        10876    10881    +5     
  Misses        737      737           

Flag	Coverage Δ
linux	93.51% <100.00%> (+<0.01%)	⬆️
pypy	93.65% <100.00%> (+<0.01%)	⬆️
windows	93.63% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
astroid/manager.py	89.75% <100.00%> (ø)
astroid/modutils.py	89.67% <100.00%> (+0.18%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[DanielNoord]

I like it!

@jacobtylerwalls generally is quite knowledgeable about our handling of imports. Do you have time for a review as well?

[yangfan-yf-yf]

The location check is the right boundary, but the regression currently only exercises a temporary directory outside both library roots. It does not cover the security-relevant branch that rejects EXT_LIB_DIRS before accepting a path under STD_LIB_DIRS.

In a virtual environment, site-packages can sit below the standard-library directory, which is exactly why the early EXT_LIB_DIRS rejection is needed. Please add a focused regression for a location under EXT_LIB_DIRS (for example, an extension-suffixed colorsys path) and assert that it is not accepted as a standard-library location. That both guards the virtualenv case and covers the currently missed branch.

I verified locally that the proposed predicate rejects such a path, and the existing temporary-directory regression passes.

[kali834x]

Good point, that branch was untested. Added IsStdLibPathTest in tests/test_modutils.py: it builds a colorsys extension path under each EXT_LIB_DIRS entry and asserts is_stdlib_path rejects it, plus a positive assertion on a real stdlib file so the check isn't trivially true.

Confirmed it fails if the EXT_LIB_DIRS rejection is dropped (site-packages resolves under the stdlib dir here), so the virtualenv case is covered now.

[yangfan-yf-yf]

Thanks for adding both requested boundary tests. I rechecked the current head: the focused modutils/manager selection passes (3 passed), Ruff passes, and the hosted checks have no failures.

The branch is currently behind main and GitHub reports it as conflicting. The only merge conflict I find against current main is in ChangeLog; the product and test files merge cleanly. Could you update the branch and resolve that changelog conflict? I will re-review the resulting head afterward.

[DanielNoord]

Needs a rebase :)

[kali834x]

Rebased onto main and resolved the ChangeLog conflict. The new entry now sits under the 4.4.0 section alongside the data-descriptor fix that landed there. Product and test files were unchanged by the rebase, and the focused modutils/manager tests still pass.

[jacobtylerwalls]

Do you have time for a review as well?

I can try over the next few days, sorry!

[jacobtylerwalls]

Thanks for working on this.

[jacobtylerwalls]

Nothing fails when I remove and location is not None, so we must be missing a test case for when location is None, e.g. for a stdlib c module like time.

[kali834x]

Right, that branch was uncovered. The reason nothing fails with a real module is that time is a statically linked builtin (C_BUILTIN, location None), so it never reaches _can_load_extension, which is only called for C_EXTENSION. The guard is there because found_spec.location is typed str | None and is_stdlib_path calls os.path.realpath, which raises on None.

Pushed a focused test that calls _can_load_extension("colorsys", None) directly and asserts False. Drop the guard and it errors with TypeError instead of returning False, so the branch is pinned now.

[jacobtylerwalls]

This test fails when I change this to time, although it's not clear to me if if the shadowing file is the one that's interpreted by astroid or not.

Can you walk me through this?

[kali834x]

Good question, and the difference is the point of picking colorsys here. colorsys is a pure-Python stdlib module, so a colorsys.<ext>.so planted at the front of sys.path resolves ahead of the real colorsys.py. astroid sees a C_EXTENSION whose location is the planted file, and that is the file it would import, which is the bug. The fix stubs it instead.

time behaves differently because it is a statically linked builtin. BuiltinImporter sits ahead of the path-based finders, so it resolves time before sys.path is ever scanned, and the planted time.so is ignored. astroid gets a C_BUILTIN with location None, never reaches the extension gate, and imports the real builtin, so nothing is stubbed and the assertion fails. The shadowing file is not what astroid interprets for time, which is why the test uses a pure-Python stdlib name.

[jacobtylerwalls]

LGTM!

[Pierre-Sassoulas]

Let's fix pre-commit and then merge :)

[kali834x]

The pre-commit failure was the merge conflict: main moved the changelog to towncrier since the last rebase, so pre-commit.ci couldn't compute a merge. Rebased again and moved the entry from ChangeLog into doc/whatsnew/fragments/3193.bugfix. pre-commit passes locally on the diff, no other changes.