Skip to content

Commit 53063b7

Browse files
authored
[3.10] gh-99240: Fix double-free bug in Argument Clinic str_converter generated code (GH-99241) (#100353)
(cherry picked from commit 8dbe08e) Fix double-free bug mentioned at GH-99240, by moving memory clean up out of "exit" label.
1 parent b81d1c3 commit 53063b7

File tree

6 files changed

+202
-25
lines changed

6 files changed

+202
-25
lines changed

Lib/test/clinic.test

+11-22
Original file line numberDiff line numberDiff line change
@@ -1740,37 +1740,26 @@ test_str_converter_encoding(PyObject *module, PyObject *const *args, Py_ssize_t
17401740
goto exit;
17411741
}
17421742
return_value = test_str_converter_encoding_impl(module, a, b, c, d, d_length, e, e_length);
1743+
/* Post parse cleanup for a */
1744+
PyMem_FREE(a);
1745+
/* Post parse cleanup for b */
1746+
PyMem_FREE(b);
1747+
/* Post parse cleanup for c */
1748+
PyMem_FREE(c);
1749+
/* Post parse cleanup for d */
1750+
PyMem_FREE(d);
1751+
/* Post parse cleanup for e */
1752+
PyMem_FREE(e);
17431753

17441754
exit:
1745-
/* Cleanup for a */
1746-
if (a) {
1747-
PyMem_FREE(a);
1748-
}
1749-
/* Cleanup for b */
1750-
if (b) {
1751-
PyMem_FREE(b);
1752-
}
1753-
/* Cleanup for c */
1754-
if (c) {
1755-
PyMem_FREE(c);
1756-
}
1757-
/* Cleanup for d */
1758-
if (d) {
1759-
PyMem_FREE(d);
1760-
}
1761-
/* Cleanup for e */
1762-
if (e) {
1763-
PyMem_FREE(e);
1764-
}
1765-
17661755
return return_value;
17671756
}
17681757

17691758
static PyObject *
17701759
test_str_converter_encoding_impl(PyObject *module, char *a, char *b, char *c,
17711760
char *d, Py_ssize_clean_t d_length, char *e,
17721761
Py_ssize_clean_t e_length)
1773-
/*[clinic end generated code: output=f579dd9e795a364e input=eb4c38e1f898f402]*/
1762+
/*[clinic end generated code: output=2d7e8b6203db31aa input=eb4c38e1f898f402]*/
17741763

17751764

17761765
/*[clinic input]

Lib/test/test_clinic.py

+16
Original file line numberDiff line numberDiff line change
@@ -1045,6 +1045,17 @@ def test_str_converter(self):
10451045
self.assertEqual(ac_tester.str_converter('a', b'b', b'c'), ('a', 'b', 'c'))
10461046
self.assertEqual(ac_tester.str_converter('a', b'b', 'c\0c'), ('a', 'b', 'c\0c'))
10471047

1048+
def test_str_converter_encoding(self):
1049+
with self.assertRaises(TypeError):
1050+
ac_tester.str_converter_encoding(1)
1051+
self.assertEqual(ac_tester.str_converter_encoding('a', 'b', 'c'), ('a', 'b', 'c'))
1052+
with self.assertRaises(TypeError):
1053+
ac_tester.str_converter_encoding('a', b'b\0b', 'c')
1054+
self.assertEqual(ac_tester.str_converter_encoding('a', b'b', bytearray([ord('c')])), ('a', 'b', 'c'))
1055+
self.assertEqual(ac_tester.str_converter_encoding('a', b'b', bytearray([ord('c'), 0, ord('c')])),
1056+
('a', 'b', 'c\x00c'))
1057+
self.assertEqual(ac_tester.str_converter_encoding('a', b'b', b'c\x00c'), ('a', 'b', 'c\x00c'))
1058+
10481059
def test_py_buffer_converter(self):
10491060
with self.assertRaises(TypeError):
10501061
ac_tester.py_buffer_converter('a', 'b')
@@ -1211,6 +1222,11 @@ def test_keyword_only_parameter(self):
12111222
ac_tester.keyword_only_parameter(1)
12121223
self.assertEqual(ac_tester.keyword_only_parameter(a=1), (1,))
12131224

1225+
def test_gh_99240_double_free(self):
1226+
expected_error = r'gh_99240_double_free\(\) argument 2 must be encoded string without null bytes, not str'
1227+
with self.assertRaisesRegex(TypeError, expected_error):
1228+
ac_tester.gh_99240_double_free('a', '\0b')
1229+
12141230

12151231
if __name__ == "__main__":
12161232
unittest.main()
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix double-free bug in Argument Clinic ``str_converter`` by
2+
extracting memory clean up to a new ``post_parsing`` section.

Modules/_testclinic.c

+79
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,64 @@ str_converter_impl(PyObject *module, const char *a, const char *b,
551551
}
552552

553553

554+
/*[clinic input]
555+
str_converter_encoding
556+
557+
a: str(encoding="idna")
558+
b: str(encoding="idna", accept={bytes, bytearray, str})
559+
c: str(encoding="idna", accept={bytes, bytearray, str}, zeroes=True)
560+
/
561+
562+
[clinic start generated code]*/
563+
564+
static PyObject *
565+
str_converter_encoding_impl(PyObject *module, char *a, char *b, char *c,
566+
Py_ssize_clean_t c_length)
567+
/*[clinic end generated code: output=ebef1a3f9b730a24 input=0c5cf5159d0e870d]*/
568+
{
569+
assert(!PyErr_Occurred());
570+
PyObject *out[3] = {NULL,};
571+
int i = 0;
572+
PyObject *arg;
573+
574+
arg = PyUnicode_FromString(a);
575+
assert(arg || PyErr_Occurred());
576+
if (!arg) {
577+
goto error;
578+
}
579+
out[i++] = arg;
580+
581+
arg = PyUnicode_FromString(b);
582+
assert(arg || PyErr_Occurred());
583+
if (!arg) {
584+
goto error;
585+
}
586+
out[i++] = arg;
587+
588+
arg = PyUnicode_FromStringAndSize(c, c_length);
589+
assert(arg || PyErr_Occurred());
590+
if (!arg) {
591+
goto error;
592+
}
593+
out[i++] = arg;
594+
595+
PyObject *tuple = PyTuple_New(3);
596+
if (!tuple) {
597+
goto error;
598+
}
599+
for (int j = 0; j < 3; j++) {
600+
PyTuple_SET_ITEM(tuple, j, out[j]);
601+
}
602+
return tuple;
603+
604+
error:
605+
for (int j = 0; j < i; j++) {
606+
Py_DECREF(out[j]);
607+
}
608+
return NULL;
609+
}
610+
611+
554612
static PyObject *
555613
bytes_from_buffer(Py_buffer *buf)
556614
{
@@ -892,6 +950,25 @@ keyword_only_parameter_impl(PyObject *module, PyObject *a)
892950
}
893951

894952

953+
/*[clinic input]
954+
gh_99240_double_free
955+
956+
a: str(encoding="idna")
957+
b: str(encoding="idna")
958+
/
959+
960+
Proof-of-concept of GH-99240 double-free bug.
961+
962+
[clinic start generated code]*/
963+
964+
static PyObject *
965+
gh_99240_double_free_impl(PyObject *module, char *a, char *b)
966+
/*[clinic end generated code: output=586dc714992fe2ed input=23db44aa91870fc7]*/
967+
{
968+
Py_RETURN_NONE;
969+
}
970+
971+
895972
static PyMethodDef tester_methods[] = {
896973
TEST_EMPTY_FUNCTION_METHODDEF
897974
OBJECTS_CONVERTER_METHODDEF
@@ -916,6 +993,7 @@ static PyMethodDef tester_methods[] = {
916993
DOUBLE_CONVERTER_METHODDEF
917994
PY_COMPLEX_CONVERTER_METHODDEF
918995
STR_CONVERTER_METHODDEF
996+
STR_CONVERTER_ENCODING_METHODDEF
919997
PY_BUFFER_CONVERTER_METHODDEF
920998
KEYWORDS_METHODDEF
921999
KEYWORDS_KWONLY_METHODDEF
@@ -933,6 +1011,7 @@ static PyMethodDef tester_methods[] = {
9331011
POSONLY_KEYWORDS_OPT_KWONLY_OPT_METHODDEF
9341012
POSONLY_OPT_KEYWORDS_OPT_KWONLY_OPT_METHODDEF
9351013
KEYWORD_ONLY_PARAMETER_METHODDEF
1014+
GH_99240_DOUBLE_FREE_METHODDEF
9361015
{NULL, NULL}
9371016
};
9381017

Modules/clinic/_testclinic.c.h

+71-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Tools/clinic/clinic.py

+23-2
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,12 @@ def __init__(self):
346346
# "goto exit" if there are any.
347347
self.return_conversion = []
348348

349+
# The C statements required to do some operations
350+
# after the end of parsing but before cleaning up.
351+
# These operations may be, for example, memory deallocations which
352+
# can only be done without any error happening during argument parsing.
353+
self.post_parsing = []
354+
349355
# The C statements required to clean up after the impl call.
350356
self.cleanup = []
351357

@@ -751,6 +757,7 @@ def parser_body(prototype, *fields, declarations=''):
751757
{modifications}
752758
{return_value} = {c_basename}_impl({impl_arguments});
753759
{return_conversion}
760+
{post_parsing}
754761
755762
{exit_label}
756763
{cleanup}
@@ -1343,6 +1350,7 @@ def render_function(self, clinic, f):
13431350
template_dict['impl_parameters'] = ", ".join(data.impl_parameters)
13441351
template_dict['impl_arguments'] = ", ".join(data.impl_arguments)
13451352
template_dict['return_conversion'] = format_escape("".join(data.return_conversion).rstrip())
1353+
template_dict['post_parsing'] = format_escape("".join(data.post_parsing).rstrip())
13461354
template_dict['cleanup'] = format_escape("".join(data.cleanup))
13471355
template_dict['return_value'] = data.return_value
13481356

@@ -1367,6 +1375,7 @@ def render_function(self, clinic, f):
13671375
return_conversion=template_dict['return_conversion'],
13681376
initializers=template_dict['initializers'],
13691377
modifications=template_dict['modifications'],
1378+
post_parsing=template_dict['post_parsing'],
13701379
cleanup=template_dict['cleanup'],
13711380
)
13721381

@@ -2595,6 +2604,10 @@ def _render_non_self(self, parameter, data):
25952604
# parse_arguments
25962605
self.parse_argument(data.parse_arguments)
25972606

2607+
# post_parsing
2608+
if post_parsing := self.post_parsing():
2609+
data.post_parsing.append('/* Post parse cleanup for ' + name + ' */\n' + post_parsing.rstrip() + '\n')
2610+
25982611
# cleanup
25992612
cleanup = self.cleanup()
26002613
if cleanup:
@@ -2686,6 +2699,14 @@ def modify(self):
26862699
"""
26872700
return ""
26882701

2702+
def post_parsing(self):
2703+
"""
2704+
The C statements required to do some operations after the end of parsing but before cleaning up.
2705+
Return a string containing this code indented at column 0.
2706+
If no operation is necessary, return an empty string.
2707+
"""
2708+
return ""
2709+
26892710
def cleanup(self):
26902711
"""
26912712
The C statements required to clean up after this variable.
@@ -3278,10 +3299,10 @@ def converter_init(self, *, accept={str}, encoding=None, zeroes=False):
32783299
if NoneType in accept and self.c_default == "Py_None":
32793300
self.c_default = "NULL"
32803301

3281-
def cleanup(self):
3302+
def post_parsing(self):
32823303
if self.encoding:
32833304
name = self.name
3284-
return "".join(["if (", name, ") {\n PyMem_FREE(", name, ");\n}\n"])
3305+
return f"PyMem_FREE({name});\n"
32853306

32863307
def parse_arg(self, argname, displayname):
32873308
if self.format_unit == 's':

0 commit comments

Comments
 (0)