Issue #6012: Add cleanup support to O& argument parsing.

Doc/c-api/arg.rst

@@ -250,6 +250,14 @@ variable(s) whose address should be passed.
    the conversion has failed.  When the conversion fails, the *converter* function
    should raise an exception and leave the content of *address* unmodified.
 
+   If the *converter* returns Py_CLEANUP_SUPPORTED, it may get called a second time
+   if the argument parsing eventually fails, giving the converter a chance to release
+   any memory that it had already allocated. In this second call, the *object* parameter
+   will be NULL; *address* will have the same value as in the original call.
+
+   .. versionchanged:: 3.1
+      Py_CLEANUP_SUPPORTED was added.
+
 ``S`` (string) [PyStringObject \*]
    Like ``O`` but requires that the Python object is a string object.  Raises
    :exc:`TypeError` if the object is not a string object.  The C variable may also

Doc/c-api/unicode.rst

@@ -379,11 +379,13 @@ Many of the following APIs take two arguments encoding and errors. These
 parameters encoding and errors have the same semantics as the ones of the
 builtin unicode() Unicode object constructor.
 
-Setting encoding to *NULL* causes the default encoding to be used which is
-ASCII.  The file system calls should use :cdata:`Py_FileSystemDefaultEncoding`
-as the encoding for file names. This variable should be treated as read-only: On
-some systems, it will be a pointer to a static string, on others, it will change
-at run-time (such as when the application invokes setlocale).
+Setting encoding to *NULL* causes the default encoding to be used
+which is ASCII.  The file system calls should use
+:cfunc:`PyUnicode_FSConverter` for encoding file names. This uses the
+variable :cdata:`Py_FileSystemDefaultEncoding` internally. This
+variable should be treated as read-only: On some systems, it will be a
+pointer to a static string, on others, it will change at run-time
+(such as when the application invokes setlocale).
 
 Error handling is set by errors which may also be set to *NULL* meaning to use
 the default handling defined for the codec.  Default error handling for all
@@ -782,6 +784,19 @@ the user settings on the machine running the codec.
    object.  Error handling is "strict".  Return *NULL* if an exception was
    raised by the codec.
 
+For decoding file names and other environment strings, :cdata:`Py_FileSystemEncoding`
+should be used as the encoding, and ``"surrogateescape"`` should be used as the error
+handler. For encoding file names during argument parsing, the ``O&`` converter should
+be used, passsing PyUnicode_FSConverter as the conversion function:
+
+.. cfunction:: int PyUnicode_FSConverter(PyObject* obj, void* result)
+
+   Convert *obj* into *result*, using the file system encoding, and the ``surrogateescape``
+   error handler. *result* must be a ``PyObject*``, yielding a bytes or bytearray object
+   which must be released if it is no longer used.
+
+   .. versionadded:: 3.1
+
 .. % --- Methods & Slots ----------------------------------------------------
 
 

Include/modsupport.h

@@ -43,6 +43,8 @@ PyAPI_FUNC(int) PyModule_AddStringConstant(PyObject *, const char *, const char
 #define PyModule_AddIntMacro(m, c) PyModule_AddIntConstant(m, #c, c)
 #define PyModule_AddStringMacro(m, c) PyModule_AddStringConstant(m, #c, c)
 
+#define Py_CLEANUP_SUPPORTED 0x20000
+
 #define PYTHON_API_VERSION 1013
 #define PYTHON_API_STRING "1013"
 /* The API version is maintained (independently from the Python version)

Lib/test/test_capi.py

@@ -115,6 +115,10 @@ class TestPendingCalls(unittest.TestCase):
         self.pendingcalls_submit(l, n)
         self.pendingcalls_wait(l, n)
 
+# Bug #6012
+class Test6012(unittest.TestCase):
+    def test(self):
+        self.assertEqual(_testcapi.argparsing("Hello", "World"), 1)
 
 def test_main():
     support.run_unittest(CAPITest)
@@ -159,7 +163,7 @@ def test_main():
         t.start()
         t.join()
 
-    support.run_unittest(TestPendingCalls)
+    support.run_unittest(TestPendingCalls, Test6012)
 
 
 if __name__ == "__main__":

Misc/NEWS

@@ -12,6 +12,8 @@ What's New in Python 3.1 release candidate 1?
 Core and Builtins
 -----------------
 
+- Issue #6012: Add cleanup support to O& argument parsing.
+
 - Issue #6089: Fixed str.format with certain invalid field specifiers
   that would raise SystemError.
 

Modules/_testcapimodule.c

@@ -1415,6 +1415,36 @@ raise_memoryerror(PyObject *self)
 	return NULL;
 }
 
+/* Issue 6012 */
+static PyObject *str1, *str2;
+static int
+failing_converter(PyObject *obj, void *arg)
+{
+	/* Clone str1, then let the conversion fail. */
+	assert(str1);
+	str2 = str1;
+	Py_INCREF(str2);
+	return 0;
+}
+static PyObject*
+argparsing(PyObject *o, PyObject *args)
+{
+	PyObject *res;
+	str1 = str2 = NULL;
+	if (!PyArg_ParseTuple(args, "O&O&",
+			      PyUnicode_FSConverter, &str1,
+			      failing_converter, &str2)) {
+		if (!str2)
+			/* argument converter not called? */
+			return NULL;
+		/* Should be 1 */
+		res = PyLong_FromLong(Py_REFCNT(str2));
+		Py_DECREF(str2);
+		PyErr_Clear();
+		return res;
+	}
+	Py_RETURN_NONE;
+}
 
 static PyMethodDef TestMethods[] = {
 	{"raise_exception",	raise_exception,		 METH_VARARGS},
@@ -1433,7 +1463,6 @@ static PyMethodDef TestMethods[] = {
 	 PyDoc_STR("This is a pretty normal docstring.")},
 	{"test_string_to_double", (PyCFunction)test_string_to_double, METH_NOARGS},
 	{"test_capsule", (PyCFunction)test_capsule, METH_NOARGS},
-
 	{"getargs_tuple",	getargs_tuple,			 METH_VARARGS},
 	{"getargs_keywords", (PyCFunction)getargs_keywords, 
 	  METH_VARARGS|METH_KEYWORDS},
@@ -1468,6 +1497,7 @@ static PyMethodDef TestMethods[] = {
 #endif
 	{"traceback_print", traceback_print, 	         METH_VARARGS},
 	{"exception_print", exception_print, 	         METH_VARARGS},
+	{"argparsing",     argparsing, METH_VARARGS},
 	{NULL, NULL} /* sentinel */
 };
 

Modules/posixmodule.c

@@ -804,8 +804,6 @@ posix_2str(PyObject *args,
 	if (!PyArg_ParseTuple(args, format,
 	                      PyUnicode_FSConverter, &opath1,
 	                      PyUnicode_FSConverter, &opath2)) {
-		Py_XDECREF(opath1);
-		Py_XDECREF(opath2);
 		return NULL;
 	}
 	path1 = bytes2str(opath1, 1);

Objects/unicodeobject.c

@@ -1539,6 +1539,10 @@ PyUnicode_FSConverter(PyObject* arg, void* addr)
     PyObject *output = NULL;
     Py_ssize_t size;
     void *data;
+    if (arg == NULL) {
+        Py_DECREF(*(PyObject**)addr);
+        return 1;
+    }
     if (PyBytes_Check(arg) || PyByteArray_Check(arg)) {
         output = arg;
         Py_INCREF(output);
@@ -1573,7 +1577,7 @@ PyUnicode_FSConverter(PyObject* arg, void* addr)
         return 0;
     }
     *(PyObject**)addr = output;
-    return 1;
+    return Py_CLEANUP_SUPPORTED;
 }
 
 

Python/getargs.c

@@ -141,6 +141,7 @@ _PyArg_VaParse_SizeT(PyObject *args, char *format, va_list va)
 
 #define GETARGS_CAPSULE_NAME_CLEANUP_PTR "getargs.cleanup_ptr"
 #define GETARGS_CAPSULE_NAME_CLEANUP_BUFFER "getargs.cleanup_buffer"
+#define GETARGS_CAPSULE_NAME_CLEANUP_CONVERT "getargs.cleanup_convert"
 
 static void
 cleanup_ptr(PyObject *self)
@@ -194,6 +195,46 @@ addcleanup(void *ptr, PyObject **freelist, PyCapsule_Destructor destr)
 	return 0;
 }
 
+static void
+cleanup_convert(PyObject *self)
+{
+	typedef int (*destr_t)(PyObject *, void *);
+	destr_t destr = (destr_t)PyCapsule_GetContext(self);
+	void *ptr = PyCapsule_GetPointer(self,
+					 GETARGS_CAPSULE_NAME_CLEANUP_CONVERT);
+	if (ptr && destr)
+		destr(NULL, ptr);
+}
+
+static int
+addcleanup_convert(void *ptr, PyObject **freelist, int (*destr)(PyObject*,void*))
+{
+	PyObject *cobj;
+	if (!*freelist) {
+		*freelist = PyList_New(0);
+		if (!*freelist) {
+			destr(NULL, ptr);
+			return -1;
+		}
+	}
+	cobj = PyCapsule_New(ptr, GETARGS_CAPSULE_NAME_CLEANUP_CONVERT, 
+			     cleanup_convert);
+	if (!cobj) {
+		destr(NULL, ptr);
+		return -1;
+	}
+	if (PyCapsule_SetContext(cobj, destr) == -1) {
+		/* This really should not happen. */
+		Py_FatalError("capsule refused setting of context.");
+	}
+	if (PyList_Append(*freelist, cobj)) {
+		Py_DECREF(cobj); /* This will also call destr. */
+		return -1;
+	}
+        Py_DECREF(cobj);
+	return 0;
+}
+
 static int
 cleanreturn(int retval, PyObject *freelist)
 {
@@ -1253,10 +1294,15 @@ convertsimple(PyObject *arg, const char **p_format, va_list *p_va, int flags,
 			typedef int (*converter)(PyObject *, void *);
 			converter convert = va_arg(*p_va, converter);
 			void *addr = va_arg(*p_va, void *);
+			int res;
 			format++;
-			if (! (*convert)(arg, addr))
+			if (! (res = (*convert)(arg, addr)))
 				return converterr("(unspecified)",
 						  arg, msgbuf, bufsize);
+			if (res == Py_CLEANUP_SUPPORTED &&
+			    addcleanup_convert(addr, freelist, convert) == -1)
+				return converterr("(cleanup problem)",
+						arg, msgbuf, bufsize);
 		}
 		else {
 			p = va_arg(*p_va, PyObject **);