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 [email protected]

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 <[email protected]>

Co-authored-by: Eric Snow <[email protected]> Signed-off-by: Filipe Laíns <[email protected]>

Signed-off-by: Filipe Laíns <[email protected]>

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-12T19:14:15Z

Python/import.c

@@ -1202,11 +1204,13 @@ set_frozen_error(frozen_status status, PyObject *modname)
            Py_UNREACHABLE();
    }
    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-12T19:14:15Z

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:14:15Z

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:14:15Z

Python/import.c

@@ -2146,7 +2147,8 @@ _imp_find_frozen_impl(PyObject *module, PyObject *name, int withdata)
        Py_RETURN_NONE;
    }
    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!

FFY00 requested review from brettcannon, encukou, ericsnowcurrently, ncoghlan and warsaw as code owners Oct 22, 2021

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 changes

Python/import.c 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 Oct 31, 2021

Apply suggestions from code review

9284de6

Co-authored-by: Eric Snow <[email protected]> Signed-off-by: Filipe Laíns <[email protected]>

add whatsnew porting entry

e22b0fd

Signed-off-by: Filipe Laíns <[email protected]>

review feedback

77b1e88

Signed-off-by: Filipe Laíns <[email protected]>

fix typo

c72d507

Signed-off-by: Filipe Laíns <[email protected]>

fix error string

fdfc83b

Signed-off-by: Filipe Laíns <[email protected]>

better error message

05811c1

Signed-off-by: Filipe Laíns <[email protected]>

fix tests

7de5615

Signed-off-by: Filipe Laíns <[email protected]>

FFY00 force-pushed the bpo-45379-bad-name branch from bddec7f to 7de5615 Compare Oct 31, 2021

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 Nov 5, 2021

ericsnowcurrently requested changes Jan 12, 2022

View changes

ericsnowcurrently removed the request for review from encukou Jan 12, 2022

ericsnowcurrently removed request for ncoghlan and warsaw Jan 12, 2022

Apr	MAY	Jun
	19
2021	2022	2023

python / cpython Public

bpo-45379: raise import error on FROZEN_BAD_NAME #29180

bpo-45379: raise import error on FROZEN_BAD_NAME #29180

FFY00 commented Oct 22, 2021 •

edited by bedevere-bot

ericsnowcurrently commented Oct 23, 2021

FFY00 commented Oct 23, 2021

FFY00 commented Oct 23, 2021

ericsnowcurrently commented Oct 30, 2021

ericsnowcurrently left a comment

bedevere-bot commented Oct 30, 2021

FFY00 commented Oct 30, 2021 •

edited by bedevere-bot

FFY00 commented Oct 31, 2021 •

edited

ericsnowcurrently left a comment

ericsnowcurrently Jan 12, 2022

ericsnowcurrently Jan 12, 2022

ericsnowcurrently Jan 12, 2022

ericsnowcurrently Jan 12, 2022

ericsnowcurrently Jan 12, 2022

FFY00 commented Jan 12, 2022

ericsnowcurrently commented Jan 12, 2022

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

python / cpython Public

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

Conversation

FFY00 commented Oct 22, 2021 • edited by bedevere-bot

ericsnowcurrently commented Oct 23, 2021

FFY00 commented Oct 23, 2021

FFY00 commented Oct 23, 2021

ericsnowcurrently commented Oct 30, 2021

ericsnowcurrently left a comment

bedevere-bot commented Oct 30, 2021

FFY00 commented Oct 30, 2021 • edited by bedevere-bot

FFY00 commented Oct 31, 2021 • edited

ericsnowcurrently left a comment

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

ericsnowcurrently Jan 12, 2022

Choose a reason for hiding this comment

FFY00 commented Jan 12, 2022

ericsnowcurrently commented Jan 12, 2022

FFY00 commented Oct 22, 2021 •

edited by bedevere-bot

FFY00 commented Oct 30, 2021 •

edited by bedevere-bot

FFY00 commented Oct 31, 2021 •

edited