Gh-142174: Explicitly disallow _as_parameter_ returning tuples
#142175
Conversation
…t conversions in ctypes and add a test for the `TypeError` it raises.
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
CodeMaverick-143
changed the title
_as_parameter_ returning tuples for default conversions in ctypes and add a test for the TypeError it raises._as_parameter_ returning tuples for default conversions in ctypes and add a test for the TypeError it raises.
CodeMaverick-143
changed the title
_as_parameter_ returning tuples for default conversions in ctypes and add a test for the TypeError it raises._as_parameter_ returning tuples for default conversions in ctypes and add a test for the TypeError it raises.
CodeMaverick-143
changed the title
_as_parameter_ returning tuples for default conversions in ctypes and add a test for the TypeError it raises._as_parameter_ returning tuples
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Please don't use LLMs to generate pull requests. See our AI policy.
| if (tstate && tstate->interp) { | ||
| REFTOTAL(tstate->interp) += n; | ||
| } |
There was a problem hiding this comment.
This looks unrelated to this change and is incorrect. tstate and tstate->interp are never NULL here.
|
|
||
| __attribute__((constructor)) void load_dyld_shared_cache_contains_path(void) { | ||
| libsystem_b_handle = dlopen("/usr/lib/libSystem.B.dylib", RTLD_LAZY); | ||
| libsystem_b_handle = dlopen("/usr/lib/libSystem.B.dylib", RTLD_LAZY | RTLD_GLOBAL); |
There was a problem hiding this comment.
Hmm, this looks unrelated as well.
There was a problem hiding this comment.
Please don't put your name in the filename. If you'd like to get credit, you can add something like "Patch by Your Name." at the end of the entry.
| @@ -0,0 +1,2 @@ | |||
|
|
|||
| def test_as_parameter_tuple(self): | ||
| class Dangerous(object): | ||
| @property | ||
| def _as_parameter_(self): | ||
| return ('i', 42) | ||
|
|
||
| func = CDLL(_ctypes_test.__file__)._testfunc_p_p | ||
| func.restype = c_int | ||
| # func.argtypes = [c_void_p] # Do not set argtypes to force default conversion | ||
|
|
||
| # Should raise ArgumentError because tuples are not supported in default conversion | ||
| with self.assertRaisesRegex(ArgumentError, "argument 1: TypeError: Don't know how to convert parameter 1"): | ||
| func(Dangerous(), 0) | ||
|
|
There was a problem hiding this comment.
This test already passes on main.
|
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 And if you don't make the requested changes, you will be poked with soft cushions! |
2 participants
Remove misleading comment about tuple support in
as_parameterand add regression testModules/_ctypes/callproc.ccontained a comment (markedXXX) stating thatas_parameterallows constructing arbitrary tuples and passing them, describing this convention as "dangerous".Analysis confirms that
ctypesdoes not support returning tuples fromas_parameterfor default conversions. Attempting to do so raises aTypeError(wrapped in anArgumentError), meaning the described security risk does not exist in the current codebase.This PR includes:
Modules/_ctypes/callproc.cto avoid confusion.test_as_parameter_tuple, inLib/test/test_ctypes/test_parameters.pyto verify that returning a tuple correctly raises aTypeError, ensuring this unsafe behavior remains disabled.