From fbb4ffe47155d82f969061df4613b83326b383ad Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 7 Jan 2025 14:48:00 +0200 Subject: [PATCH 1/5] gh-127350: Add more tests for Py_fopen() --- Lib/test/test_capi/test_file.py | 22 ++++++++++++++++++---- Modules/_testcapi/clinic/file.c.h | 25 +++++++------------------ Modules/_testcapi/file.c | 8 +++++--- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/Lib/test/test_capi/test_file.py b/Lib/test/test_capi/test_file.py index 8a08a0a93eb8b7..c5d09cc2e9eb12 100644 --- a/Lib/test/test_capi/test_file.py +++ b/Lib/test/test_capi/test_file.py @@ -5,6 +5,8 @@ _testcapi = import_helper.import_module('_testcapi') +NULL = None + class CAPIFileTest(unittest.TestCase): def test_py_fopen(self): @@ -25,14 +27,21 @@ def test_py_fopen(self): os_helper.TESTFN, os.fsencode(os_helper.TESTFN), ] - # TESTFN_UNDECODABLE cannot be used to create a file on macOS/WASI. + if os_helper.TESTFN_UNDECODABLE is not None: + filenames.append(os_helper.TESTFN_UNDECODABLE) + filenames.append(os.fsdecode(os_helper.TESTFN_UNDECODABLE)) if os_helper.TESTFN_UNENCODABLE is not None: filenames.append(os_helper.TESTFN_UNENCODABLE) for filename in filenames: with self.subTest(filename=filename): try: - with open(filename, "wb") as fp: - fp.write(source) + try: + with open(filename, "wb") as fp: + fp.write(source) + except OSError: + # TESTFN_UNDECODABLE cannot be used to create a file + # on macOS/WASI. + continue data = _testcapi.py_fopen(filename, "rb") self.assertEqual(data, source[:256]) @@ -48,6 +57,10 @@ def test_py_fopen(self): # non-ASCII mode failing with "Invalid argument" with self.assertRaises(OSError): _testcapi.py_fopen(__file__, "\xe9") + with self.assertRaises(OSError): + # \x98 is invalid in cp1250, cp1251, cp1257 + # \x9d is invalid in cp1252-cp1255, cp1258 + _testcapi.py_fopen(__file__, b"\x98\x9d") # invalid filename type for invalid_type in (123, object()): @@ -60,7 +73,8 @@ def test_py_fopen(self): # On Windows, the file mode is limited to 10 characters _testcapi.py_fopen(__file__, "rt+, ccs=UTF-8") - # CRASHES py_fopen(__file__, None) + self.assertRaises(SystemError, _testcapi.py_fopen, NULL, 'rb') + # CRASHES _testcapi.py_fopen(__file__, NULL) if __name__ == "__main__": diff --git a/Modules/_testcapi/clinic/file.c.h b/Modules/_testcapi/clinic/file.c.h index 2ca21fffcef680..fddbf48071bd3b 100644 --- a/Modules/_testcapi/clinic/file.c.h +++ b/Modules/_testcapi/clinic/file.c.h @@ -14,7 +14,8 @@ PyDoc_STRVAR(_testcapi_py_fopen__doc__, {"py_fopen", _PyCFunction_CAST(_testcapi_py_fopen), METH_FASTCALL, _testcapi_py_fopen__doc__}, static PyObject * -_testcapi_py_fopen_impl(PyObject *module, PyObject *path, const char *mode); +_testcapi_py_fopen_impl(PyObject *module, PyObject *path, const char *mode, + Py_ssize_t mode_length); static PyObject * _testcapi_py_fopen(PyObject *module, PyObject *const *args, Py_ssize_t nargs) @@ -22,27 +23,15 @@ _testcapi_py_fopen(PyObject *module, PyObject *const *args, Py_ssize_t nargs) PyObject *return_value = NULL; PyObject *path; const char *mode; - - if (!_PyArg_CheckPositional("py_fopen", nargs, 2, 2)) { - goto exit; - } - path = args[0]; - if (!PyUnicode_Check(args[1])) { - _PyArg_BadArgument("py_fopen", "argument 2", "str", args[1]); - goto exit; - } Py_ssize_t mode_length; - mode = PyUnicode_AsUTF8AndSize(args[1], &mode_length); - if (mode == NULL) { - goto exit; - } - if (strlen(mode) != (size_t)mode_length) { - PyErr_SetString(PyExc_ValueError, "embedded null character"); + + if (!_PyArg_ParseStack(args, nargs, "Oz#:py_fopen", + &path, &mode, &mode_length)) { goto exit; } - return_value = _testcapi_py_fopen_impl(module, path, mode); + return_value = _testcapi_py_fopen_impl(module, path, mode, mode_length); exit: return return_value; } -/*[clinic end generated code: output=c9fe964c3e5a0c32 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=c4dc92400306c3eb input=a9049054013a1b77]*/ diff --git a/Modules/_testcapi/file.c b/Modules/_testcapi/file.c index 4bad43010fd440..d15173fc7959e5 100644 --- a/Modules/_testcapi/file.c +++ b/Modules/_testcapi/file.c @@ -14,16 +14,18 @@ module _testcapi _testcapi.py_fopen path: object - mode: str + mode: str(zeroes=True, accept={robuffer, str, NoneType}) / Call Py_fopen(), fread(256) and Py_fclose(). Return read bytes. [clinic start generated code]*/ static PyObject * -_testcapi_py_fopen_impl(PyObject *module, PyObject *path, const char *mode) -/*[clinic end generated code: output=5a900af000f759de input=d7e7b8f0fd151953]*/ +_testcapi_py_fopen_impl(PyObject *module, PyObject *path, const char *mode, + Py_ssize_t mode_length) +/*[clinic end generated code: output=69840d0cfd8b7fbb input=f3a579dd7eb60926]*/ { + NULLABLE(path); FILE *fp = Py_fopen(path, mode); if (fp == NULL) { return NULL; From 7e4d80aa14c8a1c880057dbbf593ed793c7572f2 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 7 Jan 2025 15:46:29 +0200 Subject: [PATCH 2/5] Update tests. --- Lib/test/test_capi/test_file.py | 6 ++++-- Python/fileutils.c | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_capi/test_file.py b/Lib/test/test_capi/test_file.py index c5d09cc2e9eb12..1d5d737be54b66 100644 --- a/Lib/test/test_capi/test_file.py +++ b/Lib/test/test_capi/test_file.py @@ -56,10 +56,12 @@ def test_py_fopen(self): # non-ASCII mode failing with "Invalid argument" with self.assertRaises(OSError): - _testcapi.py_fopen(__file__, "\xe9") + _testcapi.py_fopen(__file__, b"\xc2\x80") with self.assertRaises(OSError): # \x98 is invalid in cp1250, cp1251, cp1257 # \x9d is invalid in cp1252-cp1255, cp1258 + _testcapi.py_fopen(__file__, b"\xc2\x98\xc2\x9d") + with self.assertRaises(OSError): _testcapi.py_fopen(__file__, b"\x98\x9d") # invalid filename type @@ -73,7 +75,7 @@ def test_py_fopen(self): # On Windows, the file mode is limited to 10 characters _testcapi.py_fopen(__file__, "rt+, ccs=UTF-8") - self.assertRaises(SystemError, _testcapi.py_fopen, NULL, 'rb') + # CRASHES _testcapi.py_fopen(NULL, 'rb') # CRASHES _testcapi.py_fopen(__file__, NULL) diff --git a/Python/fileutils.c b/Python/fileutils.c index 6bc3a44c3c1313..043ba844ed443a 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -1768,9 +1768,9 @@ Py_fopen(PyObject *path, const char *mode) { assert(PyGILState_Check()); - if (PySys_Audit("open", "Osi", path, mode, 0) < 0) { - return NULL; - } +// if (PySys_Audit("open", "Osi", path, mode, 0) < 0) { +// return NULL; +// } FILE *f; int async_err = 0; From 2ee41169107eb95f556f34339a3a86d729d35c2d Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 7 Jan 2025 16:15:01 +0200 Subject: [PATCH 3/5] Restore audit. --- Lib/test/test_capi/test_file.py | 2 +- Python/fileutils.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_capi/test_file.py b/Lib/test/test_capi/test_file.py index 1d5d737be54b66..327015a1ea67e8 100644 --- a/Lib/test/test_capi/test_file.py +++ b/Lib/test/test_capi/test_file.py @@ -61,7 +61,7 @@ def test_py_fopen(self): # \x98 is invalid in cp1250, cp1251, cp1257 # \x9d is invalid in cp1252-cp1255, cp1258 _testcapi.py_fopen(__file__, b"\xc2\x98\xc2\x9d") - with self.assertRaises(OSError): + with self.assertRaises((UnicodeDecodeError, OSError)): _testcapi.py_fopen(__file__, b"\x98\x9d") # invalid filename type diff --git a/Python/fileutils.c b/Python/fileutils.c index 043ba844ed443a..6bc3a44c3c1313 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -1768,9 +1768,9 @@ Py_fopen(PyObject *path, const char *mode) { assert(PyGILState_Check()); -// if (PySys_Audit("open", "Osi", path, mode, 0) < 0) { -// return NULL; -// } + if (PySys_Audit("open", "Osi", path, mode, 0) < 0) { + return NULL; + } FILE *f; int async_err = 0; From f2f9ccb94547d48fd04e08478a758e78bd7d6447 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 7 Jan 2025 16:53:41 +0200 Subject: [PATCH 4/5] Fix test on WASI. --- Lib/test/test_capi/test_file.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_capi/test_file.py b/Lib/test/test_capi/test_file.py index 327015a1ea67e8..cbe5f6c9ae6346 100644 --- a/Lib/test/test_capi/test_file.py +++ b/Lib/test/test_capi/test_file.py @@ -35,14 +35,14 @@ def test_py_fopen(self): for filename in filenames: with self.subTest(filename=filename): try: - try: - with open(filename, "wb") as fp: - fp.write(source) - except OSError: - # TESTFN_UNDECODABLE cannot be used to create a file - # on macOS/WASI. - continue - + with open(filename, "wb") as fp: + fp.write(source) + except OSError: + # TESTFN_UNDECODABLE cannot be used to create a file + # on macOS/WASI. + filename = None + continue + try: data = _testcapi.py_fopen(filename, "rb") self.assertEqual(data, source[:256]) finally: From c05feae8e14210b4962835792232d6bc730f1ecc Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 7 Jan 2025 22:02:54 +0200 Subject: [PATCH 5/5] Add a comment. --- Lib/test/test_capi/test_file.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_capi/test_file.py b/Lib/test/test_capi/test_file.py index cbe5f6c9ae6346..a67a5121c4588b 100644 --- a/Lib/test/test_capi/test_file.py +++ b/Lib/test/test_capi/test_file.py @@ -61,6 +61,7 @@ def test_py_fopen(self): # \x98 is invalid in cp1250, cp1251, cp1257 # \x9d is invalid in cp1252-cp1255, cp1258 _testcapi.py_fopen(__file__, b"\xc2\x98\xc2\x9d") + # UnicodeDecodeError can come from the audit hook code with self.assertRaises((UnicodeDecodeError, OSError)): _testcapi.py_fopen(__file__, b"\x98\x9d")