From f6e1db7d529f9e0736735426936a7cc7f75e67a9 Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Sat, 24 Feb 2018 21:47:10 +0800 Subject: [PATCH 01/10] More revealing error message when non-str objects in __all__ --- Lib/test/test___all__.py | 9 +++++++++ Python/ceval.c | 10 +++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/Lib/test/test___all__.py b/Lib/test/test___all__.py index f6e82eb64ab0254..46ad2c2be28563d 100644 --- a/Lib/test/test___all__.py +++ b/Lib/test/test___all__.py @@ -104,6 +104,15 @@ def test_all(self): ignored) print('Following modules failed to be imported:', failed_imports) + def test_invalid_type_in_all(self): + for mod in sys.modules.values(): + if hasattr(mod, "__all__") and mod.__name__ != "__future__": + break + modname = mod.__name__ + mod.__all__.append(1) + with self.assertRaisesRegex(TypeError, "__all__ must be str"): + exec("from {} import *".format(modname)) + if __name__ == "__main__": unittest.main() diff --git a/Python/ceval.c b/Python/ceval.c index 1a72413c9e1522a..f5201dba6213ac1 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4849,7 +4849,15 @@ import_all_from(PyObject *locals, PyObject *v) PyErr_Clear(); break; } - if (skip_leading_underscores && PyUnicode_Check(name)) { + if (!PyUnicode_Check(name)) { + PyErr_Format(PyExc_TypeError, + "Item in __all__ must be str, not %.100s", + Py_TYPE(name)->tp_name); + Py_DECREF(name); + err = -1; + break; + } + if (skip_leading_underscores) { if (PyUnicode_READY(name) == -1) { Py_DECREF(name); err = -1; From eadbb4417422984a2d0debd2ec3c2c61dc9d5155 Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Sat, 24 Feb 2018 21:51:53 +0800 Subject: [PATCH 02/10] add NEWS entry --- .../Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst b/Misc/NEWS.d/next/Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst new file mode 100644 index 000000000000000..a78612f5030b77d --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst @@ -0,0 +1 @@ +Make error message more revealing when there are non-str objects in __all__. From 77938189d88209583398b036653eaec315664bd9 Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Sat, 24 Feb 2018 21:57:59 +0800 Subject: [PATCH 03/10] use sys.modules key as name not mod.__name__ --- Lib/test/test___all__.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Lib/test/test___all__.py b/Lib/test/test___all__.py index 46ad2c2be28563d..a4239159a10ac6a 100644 --- a/Lib/test/test___all__.py +++ b/Lib/test/test___all__.py @@ -105,13 +105,12 @@ def test_all(self): print('Following modules failed to be imported:', failed_imports) def test_invalid_type_in_all(self): - for mod in sys.modules.values(): + for name, mod in sys.modules.items(): if hasattr(mod, "__all__") and mod.__name__ != "__future__": break - modname = mod.__name__ mod.__all__.append(1) with self.assertRaisesRegex(TypeError, "__all__ must be str"): - exec("from {} import *".format(modname)) + exec("from {} import *".format(name)) if __name__ == "__main__": From bdbd0ea1b259a25324fb74375c2f8c539fd7ab3c Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Sat, 24 Feb 2018 22:04:00 +0800 Subject: [PATCH 04/10] format rst --- .../Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst b/Misc/NEWS.d/next/Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst index a78612f5030b77d..51e3d9b613d5ee6 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2018-02-24-21-51-42.bpo-32932.2cz31L.rst @@ -1 +1 @@ -Make error message more revealing when there are non-str objects in __all__. +Make error message more revealing when there are non-str objects in ``__all__``. From f1b8f91d965f4e2078c2cdfdbab33610b77e11d5 Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Sun, 25 Feb 2018 00:01:50 +0800 Subject: [PATCH 05/10] address Serhiy's comments --- Lib/test/test___all__.py | 8 -------- Lib/test/test_import/__init__.py | 21 +++++++++++++++++++++ Python/ceval.c | 13 ++++++++++++- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/Lib/test/test___all__.py b/Lib/test/test___all__.py index a4239159a10ac6a..f6e82eb64ab0254 100644 --- a/Lib/test/test___all__.py +++ b/Lib/test/test___all__.py @@ -104,14 +104,6 @@ def test_all(self): ignored) print('Following modules failed to be imported:', failed_imports) - def test_invalid_type_in_all(self): - for name, mod in sys.modules.items(): - if hasattr(mod, "__all__") and mod.__name__ != "__future__": - break - mod.__all__.append(1) - with self.assertRaisesRegex(TypeError, "__all__ must be str"): - exec("from {} import *".format(name)) - if __name__ == "__main__": unittest.main() diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index ceea79f6ad96ff7..58b09225e67b499 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -111,6 +111,27 @@ def test_from_import_missing_attr_path_is_canonical(self): self.assertIn(cm.exception.name, {'posixpath', 'ntpath'}) self.assertIsNotNone(cm.exception) + def test_from_import_star_invalid_type(self): + TESTFNnoat = TESTFN[1:] + source = TESTFNnoat + os.extsep + "py" + sys.path.insert(0, os.curdir) + try: + with open(source, "w") as f: + f.write("__all__ = [1]") + with self.assertRaisesRegex( + TypeError, f"{TESTFNnoat}.__all__ must be str"): + exec(f"from {TESTFNnoat} import *") + unload(TESTFNnoat) + with open(source, "w") as f: + f.write("import sys\nsys.modules[__name__].__dict__[1] = 1") + with self.assertRaisesRegex( + TypeError, f"{TESTFNnoat}.__dict__ must be str"): + exec(f"from {TESTFNnoat} import *") + finally: + del sys.path[0] + remove_files(TESTFNnoat) + unload(TESTFNnoat) + def test_case_sensitivity(self): # Brief digression to test that import is case-sensitive: if we got # this far, we know for sure that "random" exists. diff --git a/Python/ceval.c b/Python/ceval.c index f5201dba6213ac1..1933f4bac5ab3e9 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4817,6 +4817,7 @@ import_all_from(PyObject *locals, PyObject *v) { _Py_IDENTIFIER(__all__); _Py_IDENTIFIER(__dict__); + _Py_IDENTIFIER(__name__); PyObject *all, *dict, *name, *value; int skip_leading_underscores = 0; int pos, err; @@ -4850,9 +4851,19 @@ import_all_from(PyObject *locals, PyObject *v) break; } if (!PyUnicode_Check(name)) { + PyObject *modname = _PyObject_GetAttrId(v, &PyId___name__); + if (modname == NULL) { + Py_DECREF(name); + err = -1; + break; + } PyErr_Format(PyExc_TypeError, - "Item in __all__ must be str, not %.100s", + "%s in %U.%s must be str, not %.100s", + skip_leading_underscores ? "Key" : "Item", + modname, + skip_leading_underscores ? "__dict__" : "__all__", Py_TYPE(name)->tp_name); + Py_DECREF(modname); Py_DECREF(name); err = -1; break; From f07e5dc8f77509e81444af8ef5a1a23438a2626e Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Tue, 27 Feb 2018 00:56:21 +0800 Subject: [PATCH 06/10] addresses Serhiy's comments again :-) --- Lib/test/test_import/__init__.py | 33 ++++++++++++++++---------------- Python/ceval.c | 2 +- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 58b09225e67b499..ea46cabb34d0163 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -112,25 +112,24 @@ def test_from_import_missing_attr_path_is_canonical(self): self.assertIsNotNone(cm.exception) def test_from_import_star_invalid_type(self): - TESTFNnoat = TESTFN[1:] - source = TESTFNnoat + os.extsep + "py" - sys.path.insert(0, os.curdir) - try: - with open(source, "w") as f: - f.write("__all__ = [1]") + with _ready_to_import() as (name, path): + with open(path, 'w') as f: + f.write("__all__ = [b'invalid_type']") + globals = {} with self.assertRaisesRegex( - TypeError, f"{TESTFNnoat}.__all__ must be str"): - exec(f"from {TESTFNnoat} import *") - unload(TESTFNnoat) - with open(source, "w") as f: - f.write("import sys\nsys.modules[__name__].__dict__[1] = 1") + TypeError, f"{name}.__all__ must be str" + ): + exec(f"from {name} import *", globals) + self.assertNotIn(b"invalid_type", globals) + with _ready_to_import() as (name, path): + with open(path, 'w') as f: + f.write("globals()[b'invalid_type'] = object()") + globals = {} with self.assertRaisesRegex( - TypeError, f"{TESTFNnoat}.__dict__ must be str"): - exec(f"from {TESTFNnoat} import *") - finally: - del sys.path[0] - remove_files(TESTFNnoat) - unload(TESTFNnoat) + TypeError, f"{name}.__dict__ must be str" + ): + exec(f"from {name} import *", globals) + self.assertNotIn(b"invalid_type", globals) def test_case_sensitivity(self): # Brief digression to test that import is case-sensitive: if we got diff --git a/Python/ceval.c b/Python/ceval.c index 1933f4bac5ab3e9..ed7aa5bf14e27b3 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4858,7 +4858,7 @@ import_all_from(PyObject *locals, PyObject *v) break; } PyErr_Format(PyExc_TypeError, - "%s in %U.%s must be str, not %.100s", + "%s in %S.%s must be str, not %.100s", skip_leading_underscores ? "Key" : "Item", modname, skip_leading_underscores ? "__dict__" : "__all__", From c13c7ded590de7eae5a98bfd3bbecf32f942ba5b Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Tue, 27 Feb 2018 10:43:14 +0800 Subject: [PATCH 07/10] escape variables --- Lib/test/test_import/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index ea46cabb34d0163..606b05784afcf40 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -112,12 +112,13 @@ def test_from_import_missing_attr_path_is_canonical(self): self.assertIsNotNone(cm.exception) def test_from_import_star_invalid_type(self): + import re with _ready_to_import() as (name, path): with open(path, 'w') as f: f.write("__all__ = [b'invalid_type']") globals = {} with self.assertRaisesRegex( - TypeError, f"{name}.__all__ must be str" + TypeError, f"{re.escape(name)}\.__all__ must be str" ): exec(f"from {name} import *", globals) self.assertNotIn(b"invalid_type", globals) @@ -126,7 +127,7 @@ def test_from_import_star_invalid_type(self): f.write("globals()[b'invalid_type'] = object()") globals = {} with self.assertRaisesRegex( - TypeError, f"{name}.__dict__ must be str" + TypeError, f"{re.escape(name)}\.__dict__ must be str" ): exec(f"from {name} import *", globals) self.assertNotIn(b"invalid_type", globals) From da79ff14932e58413b6e541dc1281da9ab19caa2 Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Sat, 24 Mar 2018 14:34:29 +0800 Subject: [PATCH 08/10] check modname --- Python/ceval.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index ed7aa5bf14e27b3..ff139eaa5106ba1 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4857,12 +4857,19 @@ import_all_from(PyObject *locals, PyObject *v) err = -1; break; } - PyErr_Format(PyExc_TypeError, - "%s in %S.%s must be str, not %.100s", - skip_leading_underscores ? "Key" : "Item", - modname, - skip_leading_underscores ? "__dict__" : "__all__", - Py_TYPE(name)->tp_name); + if (!PyUnicode_Check(modname)) { + PyErr_Format(PyExc_TypeError, + "__name__ must be a string, not %.100s", + Py_TYPE(modname)->tp_name); + } + else { + PyErr_Format(PyExc_TypeError, + "%s in %S.%s must be str, not %.100s", + skip_leading_underscores ? "Key" : "Item", + modname, + skip_leading_underscores ? "__dict__" : "__all__", + Py_TYPE(name)->tp_name); + } Py_DECREF(modname); Py_DECREF(name); err = -1; From e326e49cdf38a189316a82d9241546c6d3955918 Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Sat, 24 Mar 2018 15:53:46 +0800 Subject: [PATCH 09/10] more fix --- Python/ceval.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index ff139eaa5106ba1..29994628492f5ca 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4859,12 +4859,12 @@ import_all_from(PyObject *locals, PyObject *v) } if (!PyUnicode_Check(modname)) { PyErr_Format(PyExc_TypeError, - "__name__ must be a string, not %.100s", + "module.__name__ must be a string, not %.100s", Py_TYPE(modname)->tp_name); } else { PyErr_Format(PyExc_TypeError, - "%s in %S.%s must be str, not %.100s", + "%s in %U.%s must be str, not %.100s", skip_leading_underscores ? "Key" : "Item", modname, skip_leading_underscores ? "__dict__" : "__all__", From 995012e4c5230ef7d743537b922a7453503fa82b Mon Sep 17 00:00:00 2001 From: Xiang Zhang Date: Sat, 24 Mar 2018 17:19:01 +0800 Subject: [PATCH 10/10] leave dot out --- Python/ceval.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/ceval.c b/Python/ceval.c index 29994628492f5ca..a8ea409b9ff698f 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -4859,7 +4859,7 @@ import_all_from(PyObject *locals, PyObject *v) } if (!PyUnicode_Check(modname)) { PyErr_Format(PyExc_TypeError, - "module.__name__ must be a string, not %.100s", + "module __name__ must be a string, not %.100s", Py_TYPE(modname)->tp_name); } else {