Skip to content

gh-122071: Fixed traceback leaks global code when file does not exist #122126

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.

Sign up for GitHub

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

gh-122071: Fixed traceback leaks global code when file does not exist #122126

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f1e1189
Fixed traceback leaks global code when exec
lhish Jul 22, 2024
f510454
📜🤖 Added by blurb_it.
blurb-it[bot] Jul 22, 2024
dcc5718
Merge branch 'main' into fix-issue-122071
lhish Jul 22, 2024
b40bc28
adapt for zipimporter
lhish Jul 22, 2024
a9f8458
Fixed missing file's lazycache
lhish Jul 22, 2024
0fab629
Update trace_back test
lhish Jul 22, 2024
f3a3f97
Merge branch 'main' into fix-issue-122071
lhish Jul 22, 2024
9d7594a
Merge branch 'main' into fix-issue-122071
lhish Jul 22, 2024
c28b698
Merge branch 'main' into fix-issue-122071
lhish Jul 22, 2024
17e3545
Merge branch 'main' into fix-issue-122071
lhish Aug 24, 2024
44715f2
Update the method to check the existence of the file
lhish Aug 24, 2024
1940da7
Merge branch 'main' into fix-issue-122071
lhish Aug 27, 2024
78d1a14
change description
lhish Aug 27, 2024
363758a
Apply suggestions from code review
lhish Aug 27, 2024
fa2d76b
Revert "Update trace_back test"
lhish Aug 27, 2024
7f7308a
change the implementation of get_lines
lhish Aug 27, 2024
250f026
make restriction stronger
lhish Aug 28, 2024
b2407ff
Merge branch 'main' into fix-issue-122071
lhish Oct 6, 2024
e3796bb
Apply suggestions from code review
lhish Oct 6, 2024
3005636
change return value from true to false
lhish Oct 8, 2024
787ea13
change tests to make it correct
lhish Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Lib/linecache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import importlib._bootstrap_external
"""Cache lines from Python source files.

This is intended to read lines from modules imported -- hence if a filename
is not found, it will look down the module search path for a file by
that name.
"""

__all__ = ["getline", "clearcache", "checkcache", "lazycache"]


Expand Down Expand Up @@ -177,11 +177,16 @@ def lazycache(filename, module_globals):
return False
# Try for a __loader__, if available
if module_globals and '__name__' in module_globals:
get_lines = lambda *_, **__: ""
spec = module_globals.get('__spec__')
name = getattr(spec, 'name', None) or module_globals['__name__']
loader = getattr(spec, 'loader', None)
if loader is None:
loader = module_globals.get('__loader__')
mod_file = module_globals.get('__file__')
if isinstance(loader, importlib._bootstrap_external.SourceFileLoader) and (not mod_file or (not mod_file.endswith(filename) and not mod_file.endswith('.pyc'))):
Copy link
Member

Choose a reason for hiding this comment

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

You said that returning False makes linecache["non-existant"] fail, but how does it fail exactly (namely what are the operations you do (i.e. please provide a reproducer)? is it possible to fix this case? technically, if the file does not exist linecache should just return an empty list.

If the issue is with an existing test, then the test might also need to be updated (since the logic has been changed here).

Copy link
Author

Choose a reason for hiding this comment

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

If we directly return False, then when attempting to lazycache something that meets the conditions in the newly added if statement, it won’t add a new element to linecache.cache. As a result, accessing this key later will raise a KeyError. However, if we return an empty getline instead and return True, this issue doesn’t occur. While I doubt anyone would use code like the following, this code did not produce an error before the change:

import linecache
linecache.lazycache("11", globals())
print(linecache.cache["11"])

The output is:

print(linecache.cache["11"])
      ~~~~~~~~~~~~~~~^^^^^^
KeyError: '11'

Additionally, regarding the loader, if we don’t add this sourceloader restriction, the test_loader in test_linecache will fail:

def test_loader(self):
        filename = 'scheme://path'

        for loader in (None, object(), NoSourceLoader()):
            linecache.clearcache()
            module_globals = {'__name__': 'a.b.c', '__loader__': loader}
            self.assertEqual(linecache.getlines(filename, module_globals), [])

        linecache.clearcache()
        module_globals = {'__name__': 'a.b.c', '__loader__': FakeLoader()}
        self.assertEqual(linecache.getlines(filename, module_globals),
                         ['source for a.b.c\n'])

Since module_globals does not have filename set, mod_file = module_globals.get('__file__') definitely set mod_file to None. Without this if statement, it would normally retrieve source through the loader. However, the new if condition now covers this case, which means almost all non-standard loaders might return empty results (though I haven’t exhaustively tested this).
For ZipImporter, if inspect.getsource() is used to obtain the contents of a module imported via zipimporter, an error occurs because inspect.getsource relies on linecache.getline. When zipimporter goes through lazycache, it enters the newly added if statement, resulting in an empty getline. This is because the if checks whether mod_file and file_name have the same suffix. In the case of zipimporter, mod_file ends with .pyc, while file_name ends with .py. This ultimately causes failure in test.test_zipimport.UncompressedZipImportTestCase.testGetCompiledSource.

Copy link
Member

@picnixz picnixz Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You're not meant to access the cache using .cache directly. The cache should be accessed using linecache.getlines(). So we should perhaps see whether other path need to be updated in updatecache().

Copy link
Author

Choose a reason for hiding this comment

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

I have two questions regarding your reply.

First, Since my English is not very good, I struggled to understand your final suggestion about updating other paths in updatecache(). If you meant that all direct accesses to the cache in updatecache should be replaced with linecache.getlines, I don't think that's feasible. This is because getlines itself calls updatecache, which would create a circular dependency if updatecache were modified this way.

Second, If simply returns False, it would cause two existing Linecache-related tests to fail:

Test Case 1: test_lazycache_already_cached:
This test directly accesses the cache and expects it to have content. Here’s a snippet of the test:

    def test_lazycache_already_cached(self):
        linecache.clearcache()
        lines = linecache.getlines(NONEXISTENT_FILENAME, globals())
        self.assertEqual(
            False,
            linecache.lazycache(NONEXISTENT_FILENAME, globals()))
        self.assertEqual(4, len(linecache.cache[NONEXISTENT_FILENAME]))

In this case, returning False would result in a failure because the test expects the cache to contain data.
Test Case 2: test_lazycache_smoke:
This test expects that lazycache should return True when called with a nonexistent filename. Here’s the relevant snippet:

    def test_lazycache_smoke(self):
        lines = linecache.getlines(NONEXISTENT_FILENAME, globals())
        linecache.clearcache()
        self.assertEqual(
            True, linecache.lazycache(NONEXISTENT_FILENAME, globals()))
        self.assertEqual(1, len(linecache.cache[NONEXISTENT_FILENAME]))
        # Note here that we're looking up a nonexistent filename with no
        # globals: this would error if the lazy value wasn't resolved.
        self.assertEqual(lines, linecache.getlines(NONEXISTENT_FILENAME))

Copy link
Member

Choose a reason for hiding this comment

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

First, Since my English is not very good

Don't worry, I replied a bit too fast. My English is not perfect either so don't hesistate to ask if I was unclear!

If you meant that all direct accesses to the cache in updatecache should be replaced with linecache.getlines, I don't think that's feasible

Sorry, I meant to see whether we correctly covered the cases (namely, to see if the algorithm needs to be updated because of this new logic).

For the tests, the logic could be changed. Leaking the global code is probably worse than not leaking it IMO. I'll have a better look at the lazy-caching interface and the question on non-existent filenames (maybe we could make it work).

Copy link
Author

Choose a reason for hiding this comment

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

Alright, it does seem that returning false is appropriate in this case. However, I'm not entirely sure if other direct usages of linecache.cache need to be modified beyond the linecache and test modules. My understanding is that no further changes are necessary because the other two usages occur in pyshell and timeit. In both of these cases, the cache is directly assigned to, rather than read from.

Additionally, I've adjusted two tests that were failing in my latest commit. However, I'm not completely certain if these adjustments are logically sound.

cache[filename] = (get_lines,)
return True
get_source = getattr(loader, 'get_source', None)

if name and get_source:
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core and Builtins/2024-07-22-13-58-16.gh-issue-122071.c5EQZL.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix global code leaks when reporting tracebacks in the REPL and :func:`exec`.