bpo-45379: raise import error on FROZEN_BAD_NAME #29180

FFY00 · 2021-10-22T23:56:40Z

>>> from ctypes import py_object, pythonapi as capi
>>> capi.PyImport_ImportFrozenModuleObject(py_object(None))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: Invalid frozen object name (None)

Signed-off-by: Filipe Laíns lains@riseup.net

https://bugs.python.org/issue45379

ericsnowcurrently · 2021-10-23T00:21:44Z

Note that the existing behavior has been around for a long time. So we need to be sure this change won't break people.

FFY00 · 2021-10-23T00:24:00Z

Yes, I understand. How do you propose doing that?

FFY00 · 2021-10-23T02:43:00Z

I did some detective work, it seems almost nobody uses this function, which is not really unexpected 😅. I think the chances of someone using this and passing a None or a null pointer to it are pretty low. Is there something else you'd recommend to check? I honestly have no clue of what I could do other than this.

ericsnowcurrently · 2021-10-30T18:26:26Z

FWIW, I agree that this change in behavior is extremely unlikely to break anyone.

The users of PyImport_ImportFrozenModuleObject() (and PyImport_ImportFrozenModule() and the legacy imp.init_frozen()) are undoubtedly few. Furthermore, passing a "bad" name is a particularly uncommon case. I'd expect passing in NULL or None to be handled before the call. Names that can't be encoded to UTF-8 are probably also rare. Furthermore, if errors are silenced then they are most certainly treated the same as "not found". So the overall likelihood of hitting the error is really low.

As to the severity of hitting the error, it will be clear what the problem is and how to fix it. Callers would have to add code to treat the exception as "not found" if they prefer the existing behavior but I expect they would rather have the exception (to distinguish from "not found").

I suppose it might be worth adding a paragraph to the "Porting to Python 3.11" section of Doc/whatsnew/3.11.rst, just in case.

ericsnowcurrently

Sorry for the delay, @FFY00! Thanks for working on this.

_imp_find_frozen_impl() should also be updated in the same way. When you do that, FrozenImporter.find_spec() needs to be updated to return None if it hits that ImportError. Otherwise it will cause import to fail instead of trying the next finder on sys.metapath. It would probably make sense to emit a warning in that case. (@brettcannon, what do you think?)

In find_frozen(), we call PyErr_Clear() when emitting FROZEN_BAD_NAME. (There's a long comment there about why.) Now that we're always raising ImportError, it would be helpful to users if we preserve (chain) the error here instead of clearing it.

Also, please add a test for this exception case, both for PyImport_ImportFrozenModuleObject() (via _test_embed) and _imp.find_frozen(). Clearly this case isn't tested or your change would have caused the existing test to fail. 🙂

Other than that I just have a couple comments about explicitly noting the change in behavior.

Python/import.c

Misc/NEWS.d/next/Core and Builtins/2021-10-23-00-55-22.bpo-45379.ShZAZ7.rst

bedevere-bot · 2021-10-30T18:28:05Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

FFY00 · 2021-10-30T19:01:38Z

_imp.find_frozen already validates the name. This is done by argument clinic.

cpython/Python/clinic/import.c.h

Line 193 in aae18a1

    
           _imp_find_frozen(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames)

Python 3.11.0a1+ (heads/[bpo-45379](https://bugs.python.org/issue45379)-bad-name:e225f44088, Oct 30 2021, 19:47:09) [GCC 11.1.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import _imp
>>> _imp.find_frozen(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: find_frozen() argument 1 must be str, not None

Considering this, does FrozenImporter.find_spec still need to be updated? I assume not, right? I mean, I guess we could update it, but that is not tied to any behavior change by this PR, because _imp.find_frozen stays exactly the same.

FFY00 · 2021-10-31T00:35:22Z

Nevermind, this can indeed be triggered if we don't get valid unicode.

$ ./python -c "__import__('\\udcff')"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: Invalid frozen object name ('\udcff')

``` >>> from ctypes import py_object, pythonapi as capi >>> capi.PyImport_ImportFrozenModuleObject(py_object(None)) Traceback (most recent call last): File "<stdin>", line 1, in <module> ImportError: Invalid frozen object name (None) ``` Signed-off-by: Filipe Laíns <lains@riseup.net>

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com> Signed-off-by: Filipe Laíns <lains@riseup.net>

Signed-off-by: Filipe Laíns <lains@riseup.net>

ericsnowcurrently

Thanks again for working on this. It mostly LGTM. There are just a few minor things to address before we merge this.

ericsnowcurrently · 2022-01-12T18:42:36Z

Python/import.c

    }
    if (err != NULL) {
+        PyObject *type, *value, *traceback;
+        PyErr_Fetch(&type, &value, &traceback);


We want to chain the exception. So do one of the following:

drop this line (AFAICS, PyErr_SetImportError() chains the exception if one is set)

call _PyErr_ChainExceptions(type, value, traceback) after the PyErr_SetImportError() call (line 1213)

ericsnowcurrently · 2022-01-12T18:47:01Z

Programs/_testembed.c

+    {"test_import_frozen_module_null", test_import_frozen_module_null},
+    {"test_import_frozen_module_none", test_import_frozen_module_none},


There need to be corresponding tests in Lib/test/test_embed.py.

Actually, wouldn't the relevant code be more appropriate in Modules/_testcapimodule.c and the tests in Lib/test/test_capi.py (or even Lib/test/test_import/init.py)? As a bonus, you wouldn't need to do so much in the C code that way.

Programs/_testembed.c

ericsnowcurrently · 2022-01-12T19:02:12Z

Lib/importlib/_bootstrap.py

+        try:
+            info = _call_with_frames_removed(_imp.find_frozen, fullname)
+        except ImportError:
+            return None


Users may find it helpful if we also emitted a warning here.

ericsnowcurrently · 2022-01-12T19:12:40Z

Python/import.c

    }
    else if (status == FROZEN_BAD_NAME) {
-        Py_RETURN_NONE;
+        set_frozen_error(status, name);


This should have a test in Lib/test/test_imp.py. We should also have one in one in Lib/test/test_importlib/frozen/test_finder.py, to verify FrozenImporter.find_spec() returns None in that case.

FFY00 · 2022-01-12T19:15:53Z

Thank you for reviewing, I will go over this and rebase later.

ericsnowcurrently · 2022-01-12T20:12:12Z

Sorry for the long delay!

iritkatriel · 2023-04-10T22:11:34Z

I can't reproduce on a Mac. Is this fixed now?

The example from #29180 (comment):

>>> from ctypes import py_object, pythonapi as capi
>>> capi.PyImport_ImportFrozenModuleObject(py_object(None))
0
>>> quit()

The example from #29180 (comment):

iritkatriel@Irits-MBP cpython % ./python.exe -c "__import__('\\udcff')" 
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named '\udcff'

brettcannon · 2023-05-12T18:25:36Z

@FFY00 there's some merge conflicts now.

brettcannon · 2023-05-25T18:46:50Z

Removing my review request until the merge conflicts are fixed to at least attempt to keep my review queue "small". 😉

FFY00 requested review from brettcannon, encukou, ericsnowcurrently, ncoghlan and warsaw as code owners October 22, 2021 23:56

bedevere-bot added the awaiting review label Oct 22, 2021

the-knights-who-say-ni added the CLA signed label Oct 22, 2021

ericsnowcurrently requested changes Oct 30, 2021

View reviewed changes

Python/import.c Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Core and Builtins/2021-10-23-00-55-22.bpo-45379.ShZAZ7.rst Outdated Show resolved Hide resolved

bedevere-bot removed the awaiting review label Oct 30, 2021

bedevere-bot added the awaiting changes label Oct 30, 2021

FFY00 and others added 8 commits October 31, 2021 01:39

Apply suggestions from code review

9284de6

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com> Signed-off-by: Filipe Laíns <lains@riseup.net>

add whatsnew porting entry

e22b0fd

Signed-off-by: Filipe Laíns <lains@riseup.net>

review feedback

77b1e88

Signed-off-by: Filipe Laíns <lains@riseup.net>

fix typo

c72d507

Signed-off-by: Filipe Laíns <lains@riseup.net>

fix error string

fdfc83b

Signed-off-by: Filipe Laíns <lains@riseup.net>

better error message

05811c1

Signed-off-by: Filipe Laíns <lains@riseup.net>

fix tests

7de5615

Signed-off-by: Filipe Laíns <lains@riseup.net>

FFY00 force-pushed the bpo-45379-bad-name branch from bddec7f to 7de5615 Compare October 31, 2021 00:40

ericsnowcurrently mentioned this pull request Nov 4, 2021

Freeze/link all stdlib modules imported during startup. faster-cpython/ideas#82

Closed

46 tasks

brettcannon removed their request for review November 5, 2021 22:07

ericsnowcurrently requested changes Jan 12, 2022

View reviewed changes

ericsnowcurrently removed the request for review from encukou January 12, 2022 19:15

ericsnowcurrently removed request for ncoghlan and warsaw January 12, 2022 19:15

ezio-melotti removed the CLA signed label Jul 13, 2022

thebjorn mentioned this pull request Nov 25, 2022

remove 90% of imp usage thebjorn/pydeps#160

Merged

brettcannon self-requested a review April 12, 2023 18:31

brettcannon removed their request for review May 25, 2023 18:46

		{"test_import_frozen_module_null", test_import_frozen_module_null},
		{"test_import_frozen_module_none", test_import_frozen_module_none},

Uh oh!

bpo-45379: raise import error on FROZEN_BAD_NAME #29180

Are you sure you want to change the base?

bpo-45379: raise import error on FROZEN_BAD_NAME #29180

Uh oh!

Conversation

FFY00 commented Oct 22, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Oct 23, 2021

Uh oh!

FFY00 commented Oct 23, 2021

Uh oh!

FFY00 commented Oct 23, 2021

Uh oh!

ericsnowcurrently commented Oct 30, 2021

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Oct 30, 2021

Uh oh!

FFY00 commented Oct 30, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FFY00 commented Oct 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

FFY00 commented Jan 12, 2022

Uh oh!

ericsnowcurrently commented Jan 12, 2022

Uh oh!

iritkatriel commented Apr 10, 2023

Uh oh!

brettcannon commented May 12, 2023

Uh oh!

brettcannon commented May 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

FFY00 commented Oct 22, 2021 •

edited by bedevere-bot

Loading

FFY00 commented Oct 30, 2021 •

edited by bedevere-bot

Loading

FFY00 commented Oct 31, 2021 •

edited

Loading