gh-90949: amend GH-139234 in prevision of future mitigation API (#139366)

Fix some typos left in f04bea4,
and simplify some internal functions to ease maintenance of future
mitigation APIs.

(cherry picked from commit 68a1778)

Author: picnixz

Doc/library/pyexpat.rst

@@ -282,8 +282,8 @@ common XML vulnerabilities.
    .. note::
 
       The maximum amplification factor is only considered if the threshold
-      that can be adjusted :meth:`.SetAllocTrackerActivationThreshold` is
-      exceeded.
+      that can be adjusted by :meth:`.SetAllocTrackerActivationThreshold`
+      is exceeded.
 
    .. versionadded:: next
 

Misc/NEWS.d/next/Library/2025-09-22-14-40-11.gh-issue-90949.UM35nb.rst

@@ -1,5 +1,5 @@
-Add :func:`~xml.parsers.expat.xmlparser.SetAllocTrackerActivationThreshold`
-and :func:`~xml.parsers.expat.xmlparser.SetAllocTrackerMaximumAmplification`
+Add :meth:`~xml.parsers.expat.xmlparser.SetAllocTrackerActivationThreshold`
+and :meth:`~xml.parsers.expat.xmlparser.SetAllocTrackerMaximumAmplification`
 to :ref:`xmlparser <xmlparser-objects>` objects to prevent use of
 disproportional amounts of dynamic memory from within an Expat parser.
 Patch by Bénédikt Tran.

Modules/pyexpat.c

@@ -107,7 +107,7 @@ struct HandlerInfo {
 static struct HandlerInfo handler_info[64];
 
 static int
-set_error_code(PyObject *err, enum XML_Error code)
+set_xml_error_attr_code(PyObject *err, enum XML_Error code)
 {
     PyObject *v = PyLong_FromLong((long)code);
     int ok = v != NULL && PyObject_SetAttr(err, &_Py_ID(code), v) != -1;
@@ -119,7 +119,7 @@ set_error_code(PyObject *err, enum XML_Error code)
  * false on an exception.
  */
 static int
-set_error_location(PyObject *err, const char *name, XML_Size value)
+set_xml_error_attr_location(PyObject *err, const char *name, XML_Size value)
 {
     PyObject *v = PyLong_FromSize_t((size_t)value);
     int ok = v != NULL && PyObject_SetAttrString(err, name, v) != -1;
@@ -128,42 +128,32 @@ set_error_location(PyObject *err, const char *name, XML_Size value)
 }
 
 
-static PyObject *
-format_xml_error(enum XML_Error code, XML_Size lineno, XML_Size column)
-{
-    const char *errmsg = XML_ErrorString(code);
-    PyUnicodeWriter *writer = PyUnicodeWriter_Create(strlen(errmsg) + 1);
-    if (writer == NULL) {
-        return NULL;
-    }
-    if (PyUnicodeWriter_Format(writer,
-                               "%s: line %zu, column %zu",
-                               errmsg, (size_t)lineno, (size_t)column) < 0)
-    {
-        PyUnicodeWriter_Discard(writer);
-        return NULL;
-    }
-    return PyUnicodeWriter_Finish(writer);
-}
-
 static PyObject *
 set_xml_error(pyexpat_state *state,
               enum XML_Error code, XML_Size lineno, XML_Size column,
               const char *errmsg)
 {
-    PyObject *arg = errmsg == NULL
-        ? format_xml_error(code, lineno, column)
-        : PyUnicode_FromStringAndSize(errmsg, strlen(errmsg));
+    PyObject *arg;
+    if (errmsg == NULL) {
+        arg = PyUnicode_FromFormat(
+            "%s: line %zu, column %zu",
+            XML_ErrorString(code),
+            (size_t)lineno, (size_t)column
+        );
+    }
+    else {
+        arg = PyUnicode_FromStringAndSize(errmsg, strlen(errmsg));
+    }
     if (arg == NULL) {
         return NULL;
     }
     PyObject *res = PyObject_CallOneArg(state->error, arg);
     Py_DECREF(arg);
     if (
         res != NULL
-        && set_error_code(res, code)
-        && set_error_location(res, "lineno", lineno)
-        && set_error_location(res, "offset", column)
+        && set_xml_error_attr_code(res, code)
+        && set_xml_error_attr_location(res, "lineno", lineno)
+        && set_xml_error_attr_location(res, "offset", column)
     ) {
         PyErr_SetObject(state->error, res);
     }
@@ -1147,6 +1137,50 @@ pyexpat_xmlparser_UseForeignDTD_impl(xmlparseobject *self, PyTypeObject *cls,
 }
 #endif
 
+#if XML_COMBINED_VERSION >= 20702
+static PyObject *
+set_activation_threshold(xmlparseobject *self,
+                         PyTypeObject *cls,
+                         unsigned long long threshold,
+                         XML_Bool (*setter)(XML_Parser, unsigned long long))
+{
+    assert(self->itself != NULL);
+    if (setter(self->itself, threshold) == XML_TRUE) {
+        Py_RETURN_NONE;
+    }
+    // The setter fails if self->itself is NULL (which is not possible here)
+    // or is a non-root parser, which currently only happens for parsers
+    // created by ExternalEntityParserCreate().
+    pyexpat_state *state = PyType_GetModuleState(cls);
+    return set_invalid_arg(state, self, "parser must be a root parser");
+}
+
+static PyObject *
+set_maximum_amplification(xmlparseobject *self,
+                          PyTypeObject *cls,
+                          float max_factor,
+                          XML_Bool (*setter)(XML_Parser, float))
+{
+    assert(self->itself != NULL);
+    if (setter(self->itself, max_factor) == XML_TRUE) {
+        Py_RETURN_NONE;
+    }
+    // The setter fails if self->itself is NULL (which is not possible here),
+    // is a non-root parser, which currently only happens for parsers created
+    // by ExternalEntityParserCreate(), or if 'max_factor' is NaN or < 1.0.
+    pyexpat_state *state = PyType_GetModuleState(cls);
+    // Note: Expat has no API to determine whether a parser is a root parser,
+    // and since the Expat functions for defining the various maximum allowed
+    // amplifcation factors fail when a bad parser or an out-of-range factor
+    // is given without specifying which check failed, we check whether the
+    // factor is out-of-range to improve the error message. See also gh-90949.
+    const char *message = (isnan(max_factor) || max_factor < 1.0f)
+          ? "'max_factor' must be at least 1.0"
+          : "parser must be a root parser";
+    return set_invalid_arg(state, self, message);
+}
+#endif
+
 #if XML_COMBINED_VERSION >= 20702
 /*[clinic input]
 pyexpat.xmlparser.SetAllocTrackerActivationThreshold
@@ -1166,15 +1200,10 @@ pyexpat_xmlparser_SetAllocTrackerActivationThreshold_impl(xmlparseobject *self,
                                                           unsigned long long threshold)
 /*[clinic end generated code: output=bed7e93207ba08c5 input=9c706b75c18e4ea1]*/
 {
-    assert(self->itself != NULL);
-    if (XML_SetAllocTrackerActivationThreshold(self->itself, threshold) == XML_TRUE) {
-        Py_RETURN_NONE;
-    }
-    // XML_SetAllocTrackerActivationThreshold() can only fail if self->itself
-    // is not a root parser (currently, this is equivalent to be created
-    // by ExternalEntityParserCreate()).
-    pyexpat_state *state = PyType_GetModuleState(cls);
-    return set_invalid_arg(state, self, "parser must be a root parser");
+    return set_activation_threshold(
+        self, cls, threshold,
+        XML_SetAllocTrackerActivationThreshold
+    );
 }
 #endif
 
@@ -1207,24 +1236,10 @@ pyexpat_xmlparser_SetAllocTrackerMaximumAmplification_impl(xmlparseobject *self,
                                                            float max_factor)
 /*[clinic end generated code: output=6e44bd48c9b112a0 input=918b9266b490a722]*/
 {
-    assert(self->itself != NULL);
-    if (XML_SetAllocTrackerMaximumAmplification(self->itself, max_factor) == XML_TRUE) {
-        Py_RETURN_NONE;
-    }
-    // XML_SetAllocTrackerMaximumAmplification() can fail if self->itself
-    // is not a root parser (currently, this is equivalent to be created
-    // by ExternalEntityParserCreate()) or if 'max_factor' is NaN or < 1.0.
-    //
-    // Expat does not provide a way to determine whether a parser is a root
-    // or not, nor does it provide a way to distinguish between failures in
-    // XML_SetAllocTrackerMaximumAmplification() (see gh-90949), we manually
-    // detect the factor out-of-range issue here so that users have a better
-    // error message.
-    pyexpat_state *state = PyType_GetModuleState(cls);
-    const char *message = (isnan(max_factor) || max_factor < 1.0f)
-        ? "'max_factor' must be at least 1.0"
-        : "parser must be a root parser";
-    return set_invalid_arg(state, self, message);
+    return set_maximum_amplification(
+        self, cls, max_factor,
+        XML_SetAllocTrackerMaximumAmplification
+    );
 }
 #endif