From 5e9ef9ab16a22b43301a44ac0e442fa905bee6e6 Mon Sep 17 00:00:00 2001 From: Sean Gillespie Date: Mon, 4 May 2020 21:50:49 -0700 Subject: [PATCH 1/6] bpo-13097: fix segfault in ctypes callback invocation The ctypes module allocates arguments on the stack in `ctypes_callproc` using alloca, which is problematic when large numbers of parameters are passed. Instead of crashing, this commit raises an ArgumentError if more than 1024 parameters are passed. --- Lib/ctypes/test/test_callbacks.py | 11 +++++++++++ Modules/_ctypes/callproc.c | 11 +++++++++++ 2 files changed, 22 insertions(+) diff --git a/Lib/ctypes/test/test_callbacks.py b/Lib/ctypes/test/test_callbacks.py index f622093df61da58..3bef59f6c55d666 100644 --- a/Lib/ctypes/test/test_callbacks.py +++ b/Lib/ctypes/test/test_callbacks.py @@ -287,6 +287,17 @@ def callback(check, s): self.assertEqual(s.second, check.second) self.assertEqual(s.third, check.third) + def test_callback_too_many_args(self): + def func(*args): + return (1, "abc", None) + + nargs = 2 ** 20 + proto = CFUNCTYPE(None, *(c_int,) * nargs) + cb = proto(func) + with self.assertRaises(ArgumentError): + cb(*(1,) * nargs) + + ################################################################ if __name__ == '__main__': diff --git a/Modules/_ctypes/callproc.c b/Modules/_ctypes/callproc.c index 5c1ecabd8164dc3..55206ce55b7a742 100644 --- a/Modules/_ctypes/callproc.c +++ b/Modules/_ctypes/callproc.c @@ -1072,6 +1072,11 @@ GetComError(HRESULT errcode, GUID *riid, IUnknown *pIunk) #define IS_PASS_BY_REF(x) (x > 8 || !POW2(x)) #endif +/* + * Max number of arguments _ctypes_callproc will accept. + */ +#define CTYPES_MAX_ARGCOUNT 1024 + /* * Requirements, must be ensured by the caller: * - argtuple is tuple of arguments @@ -1107,6 +1112,12 @@ PyObject *_ctypes_callproc(PPROC pProc, ++argcount; #endif + if (argcount > CTYPES_MAX_ARGCOUNT) + { + PyErr_SetString(PyExc_ArgError, "too many arguments"); + return NULL; + } + args = (struct argument *)alloca(sizeof(struct argument) * argcount); if (!args) { PyErr_NoMemory(); From b242fc31caf9a83cea580a461a7572a5de20ab87 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 6 May 2020 02:01:25 +0000 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../NEWS.d/next/Library/2020-05-06-02-01-25.bpo-13097.Wh5xSK.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2020-05-06-02-01-25.bpo-13097.Wh5xSK.rst diff --git a/Misc/NEWS.d/next/Library/2020-05-06-02-01-25.bpo-13097.Wh5xSK.rst b/Misc/NEWS.d/next/Library/2020-05-06-02-01-25.bpo-13097.Wh5xSK.rst new file mode 100644 index 000000000000000..a7f5f5882891746 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-05-06-02-01-25.bpo-13097.Wh5xSK.rst @@ -0,0 +1 @@ +``ctypes`` now raises an ``ArgumentError`` when a callback is invoked with more than 1024 arguments. \ No newline at end of file From 062c786fa3db21357ff3150b1c19054dc9d0b2c1 Mon Sep 17 00:00:00 2001 From: Sean Gillespie Date: Wed, 6 May 2020 10:33:02 -0700 Subject: [PATCH 3/6] Apply suggestions from code review Co-authored-by: Victor Stinner --- Lib/ctypes/test/test_callbacks.py | 10 +++++++--- Modules/_ctypes/callproc.c | 3 ++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Lib/ctypes/test/test_callbacks.py b/Lib/ctypes/test/test_callbacks.py index 3bef59f6c55d666..0cb1b7af7d20f00 100644 --- a/Lib/ctypes/test/test_callbacks.py +++ b/Lib/ctypes/test/test_callbacks.py @@ -289,13 +289,17 @@ def callback(check, s): def test_callback_too_many_args(self): def func(*args): - return (1, "abc", None) + return len(args) - nargs = 2 ** 20 + CTYPES_MAX_ARGCOUNT = 1024 proto = CFUNCTYPE(None, *(c_int,) * nargs) cb = proto(func) + args1 = (None,) * CTYPES_MAX_ARGCOUNT + self.assertEqual(cb(*args1), CTYPES_MAX_ARGCOUNT) + + args2 = (None,) * (CTYPES_MAX_ARGCOUNT + 1) with self.assertRaises(ArgumentError): - cb(*(1,) * nargs) + cb(*args2) ################################################################ diff --git a/Modules/_ctypes/callproc.c b/Modules/_ctypes/callproc.c index 55206ce55b7a742..e5c22e1fe357c69 100644 --- a/Modules/_ctypes/callproc.c +++ b/Modules/_ctypes/callproc.c @@ -1114,7 +1114,8 @@ PyObject *_ctypes_callproc(PPROC pProc, if (argcount > CTYPES_MAX_ARGCOUNT) { - PyErr_SetString(PyExc_ArgError, "too many arguments"); + PyErr_Format(PyExc_ArgError, "too many arguments (%zi), maximum is %i", + argcount, CTYPES_MAX_ARGCOUNT); return NULL; } From 9bdc7d914d7d0c53b407ddb69b565aa29fba1767 Mon Sep 17 00:00:00 2001 From: Sean Gillespie Date: Wed, 6 May 2020 10:42:07 -0700 Subject: [PATCH 4/6] CR feedback: expand on comment on CTYPES_MAX_ARGCOUNT --- Modules/_ctypes/callproc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Modules/_ctypes/callproc.c b/Modules/_ctypes/callproc.c index e5c22e1fe357c69..9bc28c260717dfd 100644 --- a/Modules/_ctypes/callproc.c +++ b/Modules/_ctypes/callproc.c @@ -1073,7 +1073,10 @@ GetComError(HRESULT errcode, GUID *riid, IUnknown *pIunk) #endif /* - * Max number of arguments _ctypes_callproc will accept. + * bpo-13097: Max number of arguments _ctypes_callproc will accept. + * + * This limit is enforced for the `alloca()` call in `_ctypes_callproc`, + * to avoid allocating a massive buffer on the stack. */ #define CTYPES_MAX_ARGCOUNT 1024 From 2768c97a38c3cc8bee9f121fa5c5e710af66b8cc Mon Sep 17 00:00:00 2001 From: Sean Gillespie Date: Wed, 6 May 2020 11:14:49 -0700 Subject: [PATCH 5/6] Fix test --- Lib/ctypes/test/test_callbacks.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/ctypes/test/test_callbacks.py b/Lib/ctypes/test/test_callbacks.py index 0cb1b7af7d20f00..905b1790fbfa10d 100644 --- a/Lib/ctypes/test/test_callbacks.py +++ b/Lib/ctypes/test/test_callbacks.py @@ -292,12 +292,12 @@ def func(*args): return len(args) CTYPES_MAX_ARGCOUNT = 1024 - proto = CFUNCTYPE(None, *(c_int,) * nargs) + proto = CFUNCTYPE(c_int, *(c_int,) * CTYPES_MAX_ARGCOUNT) cb = proto(func) - args1 = (None,) * CTYPES_MAX_ARGCOUNT + args1 = (1,) * CTYPES_MAX_ARGCOUNT self.assertEqual(cb(*args1), CTYPES_MAX_ARGCOUNT) - args2 = (None,) * (CTYPES_MAX_ARGCOUNT + 1) + args2 = (1,) * (CTYPES_MAX_ARGCOUNT + 1) with self.assertRaises(ArgumentError): cb(*args2) From b78c10d92f4a07c129fcab7d5d1666715a8d23bb Mon Sep 17 00:00:00 2001 From: Sean Gillespie Date: Wed, 6 May 2020 12:40:14 -0700 Subject: [PATCH 6/6] make patchcheck --- Lib/ctypes/test/test_callbacks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/ctypes/test/test_callbacks.py b/Lib/ctypes/test/test_callbacks.py index 905b1790fbfa10d..937a06d981b0034 100644 --- a/Lib/ctypes/test/test_callbacks.py +++ b/Lib/ctypes/test/test_callbacks.py @@ -296,7 +296,7 @@ def func(*args): cb = proto(func) args1 = (1,) * CTYPES_MAX_ARGCOUNT self.assertEqual(cb(*args1), CTYPES_MAX_ARGCOUNT) - + args2 = (1,) * (CTYPES_MAX_ARGCOUNT + 1) with self.assertRaises(ArgumentError): cb(*args2)