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 RuntimeError caused by analyzing live objects with __getattribute__ or descriptors #2687

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

aatle
Copy link
Contributor

@aatle aatle commented Feb 16, 2025

Type of Changes

Type
🐛 Bug fix

Description

During checking, lazy module objects may get loaded during analysis of sys.modules, causing a crash. (In general, when an object executes code from an attribute access, making its containing collection change size.)
Avoid some attribute accesses in const_factory() to avoid executing __getattribute__ code.

Closes #2686
Closes pylint-dev/pylint#8589

Copy link

codecov bot commented Feb 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.19%. Comparing base (7318c43) to head (965a213).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2687   +/-   ##
=======================================
  Coverage   93.19%   93.19%           
=======================================
  Files          93       93           
  Lines       11066    11067    +1     
=======================================
+ Hits        10313    10314    +1     
  Misses        753      753           
Flag Coverage Δ
linux 93.08% <100.00%> (+<0.01%) ⬆️
pypy 93.19% <100.00%> (+<0.01%) ⬆️
windows 93.19% <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/nodes/node_classes.py 94.98% <100.00%> (+<0.01%) ⬆️

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Is there a chance we can add a test for this? Or is that too difficult?

@@ -5514,6 +5514,7 @@ def _create_basic_elements(
"""Create a list of nodes to function as the elements of a new node."""
elements: list[NodeNG] = []
for element in value:
# NOTE: avoid accessing any attributes of element in the loop.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this comment and the one below. If the implementation of const_factory ever changes this is outdated. Imo, comments should be at the offending code (which you already did 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand.
The problem happens in the loop, so possibly it could occur directly in _create_basic_elements() without need for const_factory() to cause it.
I added the extra comments because the value is dangerous for the whole loop body (including const_factory()), until the value might be identified as const type.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

@jacobtylerwalls Anything to add?

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks!

@jacobtylerwalls jacobtylerwalls added this to the 3.3.9 milestone Feb 19, 2025
@jacobtylerwalls jacobtylerwalls merged commit 71dc6b2 into pylint-dev:main Feb 19, 2025
21 checks passed
Copy link
Contributor

The backport to maintenance/3.3.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-maintenance/3.3.x maintenance/3.3.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/3.3.x
# Create a new branch
git switch --create backport-2687-to-maintenance/3.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 71dc6b2412f95e0ad01cefdd6ee0d8e9ea551376
# Push it to GitHub
git push --set-upstream origin backport-2687-to-maintenance/3.3.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/3.3.x

Then, create a pull request where the base branch is maintenance/3.3.x and the compare/head branch is backport-2687-to-maintenance/3.3.x.

jacobtylerwalls pushed a commit to jacobtylerwalls/astroid that referenced this pull request Feb 20, 2025
…e__` or descriptors (pylint-dev#2687)

Avoid some attribute accesses in const_factory

(cherry picked from commit 71dc6b2)
@aatle aatle deleted the fix-live-attrs branch February 20, 2025 07:12
jacobtylerwalls pushed a commit that referenced this pull request Mar 2, 2025
…e__` or descriptors (#2687)

Avoid some attribute accesses in const_factory

(cherry picked from commit 71dc6b2)
jacobtylerwalls added a commit that referenced this pull request Mar 2, 2025
…e__` or descriptors (#2687) (#2688)

Avoid some attribute accesses in const_factory

(cherry picked from commit 71dc6b2)

Co-authored-by: aatle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuntimeError when analyzing sys.modules live object RuntimeError: dictionary changed size during iteration
3 participants