From fbfbec3b9b08957242b93d2d1ad789041850b738 Mon Sep 17 00:00:00 2001 From: mrmilosz Date: Sat, 24 May 2014 23:47:09 -0400 Subject: [PATCH 01/15] cursor.callproc now also accepts dict for PostgreSQL 9+ "named notation" --- psycopg/cursor_type.c | 56 ++++++++++++++++++++++++++++++++++--------- psycopg/python.h | 2 ++ 2 files changed, 47 insertions(+), 11 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 623ed128..d6bedfb7 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1015,6 +1015,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) PyObject *parameters = Py_None; PyObject *operation = NULL; PyObject *res = NULL; + PyObject *parameter_names = NULL; if (!PyArg_ParseTuple(args, "s#|O", &procname, &procname_len, ¶meters @@ -1037,19 +1038,52 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) } /* allocate some memory, build the SQL and create a PyString from it */ - sl = procname_len + 17 + nparameters*3 - (nparameters ? 1 : 0); - sql = (char*)PyMem_Malloc(sl); - if (sql == NULL) { - PyErr_NoMemory(); - goto exit; - } - sprintf(sql, "SELECT * FROM %s(", procname); - for(i=0; i 0 && PyDict_Check(parameters)) { + /* for a dict, we put the parameter names into the SQL */ + parameter_names = PyDict_Keys(parameters); + + /* first we need to figure out how much space we need for the SQL */ + sl = procname_len + 17 + nparameters*5 - (nparameters ? 1 : 0); + for(i=0; i 2 From 457491cc6faad09e35f6e0de5a64c2a507b3accb Mon Sep 17 00:00:00 2001 From: mrmilosz Date: Sun, 25 May 2014 04:03:07 -0400 Subject: [PATCH 02/15] callproc using a dict now has a type check to make sure the keys are strings. --- psycopg/cursor_type.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index d6bedfb7..67694ab4 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1015,6 +1015,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) PyObject *parameters = Py_None; PyObject *operation = NULL; PyObject *res = NULL; + PyObject *parameter_name = NULL; PyObject *parameter_names = NULL; if (!PyArg_ParseTuple(args, "s#|O", @@ -1046,7 +1047,12 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) /* first we need to figure out how much space we need for the SQL */ sl = procname_len + 17 + nparameters*5 - (nparameters ? 1 : 0); for(i=0; i Date: Wed, 28 May 2014 01:05:17 -0400 Subject: [PATCH 03/15] callproc using a dict now uses connection encoding and sanitizes parameter names --- psycopg/cursor_type.c | 57 ++++++++++++++++++++++++++++++++++--------- psycopg/python.h | 2 -- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 67694ab4..68ecb3bd 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1016,6 +1016,10 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) PyObject *operation = NULL; PyObject *res = NULL; PyObject *parameter_name = NULL; + PyObject *parameter_name_bytes = NULL; + char *parameter_name_cstr = NULL; + char *parameter_name_cstr_sanitized = NULL; + char **parameter_name_cstr_sanitized_CACHE = NULL; PyObject *parameter_names = NULL; if (!PyArg_ParseTuple(args, "s#|O", @@ -1040,40 +1044,71 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) /* allocate some memory, build the SQL and create a PyString from it */ + /* a dict requires special handling: we put the parameter names into the SQL */ if (nparameters > 0 && PyDict_Check(parameters)) { - /* for a dict, we put the parameter names into the SQL */ + parameter_names = PyDict_Keys(parameters); - /* first we need to figure out how much space we need for the SQL */ - sl = procname_len + 17 + nparameters*5 - (nparameters ? 1 : 0); + /* first we need to ensure the dict's keys are text */ for(i=0; iconn->pgconn, parameter_name_cstr, strlen(parameter_name_cstr)); + Py_DECREF(parameter_name); + + /* must add the length of the sanitized string to the length of the SQL string */ + sl += strlen(parameter_name_cstr_sanitized); + + parameter_name_cstr_sanitized_CACHE[i] = parameter_name_cstr_sanitized; + } + + Py_DECREF(parameter_names); + sql = (char*)PyMem_Malloc(sl); if (sql == NULL) { - PyErr_NoMemory(); - goto exit; + PyErr_NoMemory(); + for(i=0; i 2 From 9810d508b5d8c27c7e557088c19769170da16292 Mon Sep 17 00:00:00 2001 From: mrmilosz Date: Wed, 28 May 2014 01:27:56 -0400 Subject: [PATCH 04/15] cursor.callproc: added a missing memory check --- psycopg/cursor_type.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 68ecb3bd..8506a75b 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1064,6 +1064,13 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) /* we will throw the sanitized C strings into a cache to not redo the work later */ parameter_name_cstr_sanitized_CACHE = PyMem_New(char *, nparameters); + if (parameter_name_cstr_sanitized_CACHE == NULL) { + PyErr_NoMemory(); + PyMem_Del(parameter_name_cstr_sanitized_CACHE); + Py_DECREF(parameter_names); + goto exit; + } + for(i=0; i Date: Thu, 29 May 2014 04:31:43 -0400 Subject: [PATCH 05/15] callproc: now more compliant with local coding standards. --- psycopg/cursor_type.c | 196 ++++++++++++++++++++++-------------------- 1 file changed, 105 insertions(+), 91 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 8506a75b..fe4eb2ec 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1015,17 +1015,18 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) PyObject *parameters = Py_None; PyObject *operation = NULL; PyObject *res = NULL; - PyObject *parameter_name = NULL; - PyObject *parameter_name_bytes = NULL; - char *parameter_name_cstr = NULL; - char *parameter_name_cstr_sanitized = NULL; - char **parameter_name_cstr_sanitized_CACHE = NULL; - PyObject *parameter_names = NULL; - if (!PyArg_ParseTuple(args, "s#|O", - &procname, &procname_len, ¶meters - )) - { goto exit; } + int using_dict; + PyObject *pname = NULL; + PyObject *bpname = NULL; + PyObject *pnames = NULL; + char *cpname = NULL; + char **scpnames = NULL; + + if (!PyArg_ParseTuple(args, "s#|O", &procname, &procname_len, + ¶meters)) { + goto exit; + } EXC_IF_CURS_CLOSED(self); EXC_IF_ASYNC_IN_PROGRESS(self, callproc); @@ -1033,115 +1034,128 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) if (self->name != NULL) { psyco_set_error(ProgrammingError, self, - "can't call .callproc() on named cursors"); + "can't call .callproc() on named cursors"); goto exit; } if (parameters != Py_None) { nparameters = PyObject_Length(parameters); - if (nparameters < 0) nparameters = 0; + if (nparameters < 0) { + nparameters = 0; + } } - /* allocate some memory, build the SQL and create a PyString from it */ + using_dict = nparameters > 0 && PyDict_Check(parameters); - /* a dict requires special handling: we put the parameter names into the SQL */ - if (nparameters > 0 && PyDict_Check(parameters)) { - - parameter_names = PyDict_Keys(parameters); - - /* first we need to ensure the dict's keys are text */ - for(i=0; iconn->pgconn, parameter_name_cstr, strlen(parameter_name_cstr)); - Py_DECREF(parameter_name); - - /* must add the length of the sanitized string to the length of the SQL string */ - sl += strlen(parameter_name_cstr_sanitized); - - parameter_name_cstr_sanitized_CACHE[i] = parameter_name_cstr_sanitized; - } - - Py_DECREF(parameter_names); - - sql = (char*)PyMem_Malloc(sl); - if (sql == NULL) { - PyErr_NoMemory(); - for(i=0; iconn->pgconn, cpname, + strlen(cpname)))) { + PyErr_SetString(PyExc_RuntimeError, + "libpq failed to escape identifier!"); + goto exit; + } + + sl += strlen(scpnames[i]); + } + + sql = (char*)PyMem_Malloc(sl); + if (sql == NULL) { + PyErr_NoMemory(); + goto exit; + } + + sprintf(sql, "SELECT * FROM %s(", procname); + for (i = 0; i < nparameters; i++) { + strcat(sql, scpnames[i]); + strcat(sql, ":=%s,"); + } + sql[sl-2] = ')'; + sql[sl-1] = '\0'; + + if (!(parameters = PyDict_Values(parameters))) { + PyErr_SetString(PyExc_RuntimeError, + "built-in 'values' failed on a Dict!"); + goto exit; + } } - /* a list (or None) is a little bit simpler */ + /* a list (or None, or empty data structure) is a little bit simpler */ else { - sl = procname_len + 17 + nparameters*3 - (nparameters ? 1 : 0); + sl = procname_len + 17 + nparameters * 3 - (nparameters ? 1 : 0); - sql = (char*)PyMem_Malloc(sl); - if (sql == NULL) { - PyErr_NoMemory(); - goto exit; - } + sql = (char*)PyMem_Malloc(sl); + if (sql == NULL) { + PyErr_NoMemory(); + goto exit; + } - sprintf(sql, "SELECT * FROM %s(", procname); - for(i=0; iconn->async, 0)) { - Py_INCREF(parameters); + self->conn->async, 0)) { + /* In the dict case, the parameters are already a new reference */ + if (!using_dict) { + Py_INCREF(parameters); + } res = parameters; } exit: + if (scpnames != NULL) { + for (i = 0; i < nparameters; i++) { + if (scpnames[i] != NULL) { + PQfreemem(scpnames[i]); + } + } + } + PyMem_Del(scpnames); + Py_XDECREF(pnames); Py_XDECREF(operation); PyMem_Free((void*)sql); return res; From a02519ed4342957029c580b1c9b604c5b12e876b Mon Sep 17 00:00:00 2001 From: mrmilosz Date: Thu, 29 May 2014 05:09:09 -0400 Subject: [PATCH 06/15] callproc: checking for libpq 9.0+ on compile. yes: use PQescapeIdentifier. no: error --- psycopg/cursor_type.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index fe4eb2ec..7475be78 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1017,11 +1017,13 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) PyObject *res = NULL; int using_dict; +#if PG_VERSION_HEX >= 0x090000 PyObject *pname = NULL; PyObject *bpname = NULL; PyObject *pnames = NULL; char *cpname = NULL; char **scpnames = NULL; +#endif if (!PyArg_ParseTuple(args, "s#|O", &procname, &procname_len, ¶meters)) { @@ -1049,6 +1051,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) /* A Dict is complicated. The parameter names go into the query */ if (using_dict) { +#if PG_VERSION_HEX >= 0x090000 if (!(pnames = PyDict_Keys(parameters))) { PyErr_SetString(PyExc_RuntimeError, "built-in 'keys' failed on a Dict!"); @@ -1113,6 +1116,11 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) "built-in 'values' failed on a Dict!"); goto exit; } +#else + PyErr_SetString(PyExc_NotImplementedError, + "named parameters require psycopg2 compiled against libpq 9.0+"); + goto exit; +#endif } /* a list (or None, or empty data structure) is a little bit simpler */ @@ -1147,6 +1155,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) } exit: +#if PG_VERSION_HEX >= 0x090000 if (scpnames != NULL) { for (i = 0; i < nparameters; i++) { if (scpnames[i] != NULL) { @@ -1156,6 +1165,7 @@ exit: } PyMem_Del(scpnames); Py_XDECREF(pnames); +#endif Py_XDECREF(operation); PyMem_Free((void*)sql); return res; From d558cc5cf84408a1ed4f16dce81586df2f7f38e0 Mon Sep 17 00:00:00 2001 From: mrmilosz Date: Fri, 30 May 2014 20:34:19 -0400 Subject: [PATCH 07/15] callproc: tests, docs, and comment/error-reporting touchups. --- doc/src/cursor.rst | 17 ++++++++++------ psycopg/cursor_type.c | 36 +++++++++++++++++++++++---------- tests/test_cursor.py | 37 ++++++++++++++++++++++++++++++++++ tests/test_psycopg2_dbapi20.py | 23 +++++++++++++++++++++ 4 files changed, 96 insertions(+), 17 deletions(-) diff --git a/doc/src/cursor.rst b/doc/src/cursor.rst index ebfa90ad..168f49ec 100644 --- a/doc/src/cursor.rst +++ b/doc/src/cursor.rst @@ -201,12 +201,17 @@ The ``cursor`` class Call a stored database procedure with the given name. The sequence of parameters must contain one entry for each argument that the procedure - expects. The result of the call is returned as modified copy of the - input sequence. Input parameters are left untouched, output and - input/output parameters replaced with possibly new values. - - The procedure may also provide a result set as output. This must then - be made available through the standard |fetch*|_ methods. + expects. Overloaded procedures are supported. Named parameters can be + used with a PostgreSQL 9.0+ client by supplying the sequence of + parameters as a Dict. + + This function is, at present, not DBAPI-compliant. The return value is + supposed to consist of the sequence of parameters with modified output + and input/output parameters. In future versions, the DBAPI-compliant + return value may be implemented, but for now the function returns None. + + The procedure may provide a result set as output. This is then made + available through the standard |fetch*|_ methods. .. method:: mogrify(operation [, parameters]) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 7475be78..e12e8146 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1019,6 +1019,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) int using_dict; #if PG_VERSION_HEX >= 0x090000 PyObject *pname = NULL; + PyObject *spname = NULL; PyObject *bpname = NULL; PyObject *pnames = NULL; char *cpname = NULL; @@ -1049,7 +1050,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) using_dict = nparameters > 0 && PyDict_Check(parameters); - /* A Dict is complicated. The parameter names go into the query */ + /* a Dict is complicated; the parameter names go into the query */ if (using_dict) { #if PG_VERSION_HEX >= 0x090000 if (!(pnames = PyDict_Keys(parameters))) { @@ -1067,33 +1068,46 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) memset(scpnames, 0, sizeof(char *) * nparameters); - /* Each parameter has to be processed. It's a few steps. */ + /* each parameter has to be processed; it's a few steps. */ for (i = 0; i < nparameters; i++) { + /* all errors are RuntimeErrors as they should never occur */ + if (!(pname = PyList_GetItem(pnames, i))) { PyErr_SetString(PyExc_RuntimeError, "built-in 'values' did not return List!"); goto exit; } - if (!(bpname = psycopg_ensure_bytes(pname))) { - PyErr_SetString(PyExc_TypeError, - "argument 2 must have only string keys if Dict"); + if (!(spname = PyObject_Str(pname))) { + PyErr_SetString(PyExc_RuntimeError, + "built-in 'str' failed!"); + goto exit; + } + + /* this is the only function here that returns a new reference */ + if (!(bpname = psycopg_ensure_bytes(spname))) { + PyErr_SetString(PyExc_RuntimeError, + "failed to get Bytes from text!"); goto exit; } if (!(cpname = Bytes_AsString(bpname))) { + Py_XDECREF(bpname); PyErr_SetString(PyExc_RuntimeError, - "failed to get Bytes from String!"); + "failed to get cstr from Bytes!"); goto exit; } if (!(scpnames[i] = PQescapeIdentifier(self->conn->pgconn, cpname, strlen(cpname)))) { + Py_XDECREF(bpname); PyErr_SetString(PyExc_RuntimeError, "libpq failed to escape identifier!"); goto exit; } + Py_XDECREF(bpname); + sl += strlen(scpnames[i]); } @@ -1147,11 +1161,11 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) if (0 <= _psyco_curs_execute(self, operation, parameters, self->conn->async, 0)) { - /* In the dict case, the parameters are already a new reference */ - if (!using_dict) { - Py_INCREF(parameters); - } - res = parameters; + if (using_dict) { + Py_DECREF(parameters); + } + /* return None from this until it's DBAPI compliant... */ + res = Py_None; } exit: diff --git a/tests/test_cursor.py b/tests/test_cursor.py index cba5ccaa..c6f0e144 100755 --- a/tests/test_cursor.py +++ b/tests/test_cursor.py @@ -427,6 +427,43 @@ class CursorTests(ConnectingTestCase): self.assertRaises(psycopg2.InterfaceError, cur.executemany, 'select 1', []) + # It would be inappropriate to test callproc's named parameters in the + # DBAPI2.0 test section because they are a psycopg2 extension. + @skip_before_postgres(9, 0) + def test_callproc_dict(self): + # This parameter name tests for injection and quote escaping + paramname = ''' + Robert'); drop table "students" -- + '''.strip() + escaped_paramname = '"%s"' % paramname.replace('"', '""') + procname = 'pg_temp.randall' + + cur = self.conn.cursor() + + # Set up the temporary function + cur.execute(''' + CREATE FUNCTION %s(%s INT) + RETURNS INT AS + 'SELECT $1 * $1' + LANGUAGE SQL + ''' % (procname, escaped_paramname)); + + # Make sure callproc works right + cur.callproc(procname, { paramname: 2 }) + self.assertEquals(cur.fetchone()[0], 4) + + # Make sure callproc fails right + failing_cases = [ + ({ paramname: 2, 'foo': 'bar' }, psycopg2.ProgrammingError), + ({ paramname: '2' }, psycopg2.ProgrammingError), + ({ paramname: 'two' }, psycopg2.ProgrammingError), + ({ 'bjørn': 2 }, psycopg2.ProgrammingError), + ({ 3: 2 }, psycopg2.ProgrammingError), + ({ self: 2 }, psycopg2.ProgrammingError), + ] + for parameter_sequence, exception in failing_cases: + self.assertRaises(exception, cur.callproc, procname, parameter_sequence) + self.conn.rollback() def test_suite(): return unittest.TestLoader().loadTestsFromName(__name__) diff --git a/tests/test_psycopg2_dbapi20.py b/tests/test_psycopg2_dbapi20.py index 744d3224..28ea6690 100755 --- a/tests/test_psycopg2_dbapi20.py +++ b/tests/test_psycopg2_dbapi20.py @@ -36,6 +36,29 @@ class Psycopg2Tests(dbapi20.DatabaseAPI20Test): connect_kw_args = {'dsn': dsn} lower_func = 'lower' # For stored procedure test + def test_callproc(self): + # Until DBAPI 2.0 compliance, callproc should return None or it's just + # misleading. Therefore, we will skip the return value test for + # callproc and only perform the fetch test. + # + # For what it's worth, the DBAPI2.0 test_callproc doesn't actually + # test for DBAPI2.0 compliance! It doesn't check for modified OUT and + # IN/OUT parameters in the return values! + con = self._connect() + try: + cur = con.cursor() + if self.lower_func and hasattr(cur,'callproc'): + cur.callproc(self.lower_func,('FOO',)) + r = cur.fetchall() + self.assertEqual(len(r),1,'callproc produced no result set') + self.assertEqual(len(r[0]),1, + 'callproc produced invalid result set' + ) + self.assertEqual(r[0][0],'foo', + 'callproc produced invalid results' + ) + finally: + con.close() def test_setoutputsize(self): # psycopg2's setoutputsize() is a no-op From e53c738fec71c823a556928aadcb0c373e5e722c Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 5 Jun 2014 01:25:22 +0200 Subject: [PATCH 08/15] Avoid clobbering the exceptions raised by other calls --- psycopg/cursor_type.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index e12e8146..0ee70e69 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1054,8 +1054,6 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) if (using_dict) { #if PG_VERSION_HEX >= 0x090000 if (!(pnames = PyDict_Keys(parameters))) { - PyErr_SetString(PyExc_RuntimeError, - "built-in 'keys' failed on a Dict!"); goto exit; } @@ -1073,36 +1071,26 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) /* all errors are RuntimeErrors as they should never occur */ if (!(pname = PyList_GetItem(pnames, i))) { - PyErr_SetString(PyExc_RuntimeError, - "built-in 'values' did not return List!"); goto exit; } if (!(spname = PyObject_Str(pname))) { - PyErr_SetString(PyExc_RuntimeError, - "built-in 'str' failed!"); goto exit; } /* this is the only function here that returns a new reference */ if (!(bpname = psycopg_ensure_bytes(spname))) { - PyErr_SetString(PyExc_RuntimeError, - "failed to get Bytes from text!"); goto exit; } if (!(cpname = Bytes_AsString(bpname))) { Py_XDECREF(bpname); - PyErr_SetString(PyExc_RuntimeError, - "failed to get cstr from Bytes!"); goto exit; } if (!(scpnames[i] = PQescapeIdentifier(self->conn->pgconn, cpname, strlen(cpname)))) { Py_XDECREF(bpname); - PyErr_SetString(PyExc_RuntimeError, - "libpq failed to escape identifier!"); goto exit; } @@ -1126,8 +1114,6 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) sql[sl-1] = '\0'; if (!(parameters = PyDict_Values(parameters))) { - PyErr_SetString(PyExc_RuntimeError, - "built-in 'values' failed on a Dict!"); goto exit; } #else From 61696b0603abc365b821dae27acd9c594d824468 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 5 Jun 2014 01:32:58 +0200 Subject: [PATCH 09/15] Added guard on params with no length on callproc --- psycopg/cursor_type.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 0ee70e69..ec315338 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1042,9 +1042,8 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) } if (parameters != Py_None) { - nparameters = PyObject_Length(parameters); - if (nparameters < 0) { - nparameters = 0; + if (-1 == (nparameters = PyObject_Length(parameters))) { + goto exit; } } From 09fb8ffd9d978ecae0c8c05f4b4950fb03460017 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 5 Jun 2014 02:18:05 +0200 Subject: [PATCH 10/15] Raise TypeError if the dict in callproc param contains non-strings Check-and-conversion chain fixed and simplified. 'spname' was a reference leak. --- psycopg/cursor_type.c | 38 ++++++++++++-------------------------- tests/test_cursor.py | 6 +++--- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index ec315338..8282e661 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1019,8 +1019,6 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) int using_dict; #if PG_VERSION_HEX >= 0x090000 PyObject *pname = NULL; - PyObject *spname = NULL; - PyObject *bpname = NULL; PyObject *pnames = NULL; char *cpname = NULL; char **scpnames = NULL; @@ -1069,37 +1067,24 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) for (i = 0; i < nparameters; i++) { /* all errors are RuntimeErrors as they should never occur */ - if (!(pname = PyList_GetItem(pnames, i))) { + if (!(pname = PyList_GetItem(pnames, i))) { goto exit; } + Py_INCREF(pname); /* was borrowed */ + + /* this also makes a check for keys being strings */ + if (!(pname = psycopg_ensure_bytes(pname))) { goto exit; } + if (!(cpname = Bytes_AsString(pname))) { goto exit; } + + if (!(scpnames[i] = PQescapeIdentifier( + self->conn->pgconn, cpname, strlen(cpname)))) { goto exit; } - if (!(spname = PyObject_Str(pname))) { - goto exit; - } - - /* this is the only function here that returns a new reference */ - if (!(bpname = psycopg_ensure_bytes(spname))) { - goto exit; - } - - if (!(cpname = Bytes_AsString(bpname))) { - Py_XDECREF(bpname); - goto exit; - } - - if (!(scpnames[i] = PQescapeIdentifier(self->conn->pgconn, cpname, - strlen(cpname)))) { - Py_XDECREF(bpname); - goto exit; - } - - Py_XDECREF(bpname); + Py_CLEAR(pname); sl += strlen(scpnames[i]); } - sql = (char*)PyMem_Malloc(sl); - if (sql == NULL) { + if (!(sql = (char*)PyMem_Malloc(sl))) { PyErr_NoMemory(); goto exit; } @@ -1163,6 +1148,7 @@ exit: } } PyMem_Del(scpnames); + Py_XDECREF(pname); Py_XDECREF(pnames); #endif Py_XDECREF(operation); diff --git a/tests/test_cursor.py b/tests/test_cursor.py index c6f0e144..fc389e1a 100755 --- a/tests/test_cursor.py +++ b/tests/test_cursor.py @@ -457,9 +457,9 @@ class CursorTests(ConnectingTestCase): ({ paramname: 2, 'foo': 'bar' }, psycopg2.ProgrammingError), ({ paramname: '2' }, psycopg2.ProgrammingError), ({ paramname: 'two' }, psycopg2.ProgrammingError), - ({ 'bjørn': 2 }, psycopg2.ProgrammingError), - ({ 3: 2 }, psycopg2.ProgrammingError), - ({ self: 2 }, psycopg2.ProgrammingError), + ({ u'bj\xc3rn': 2 }, psycopg2.ProgrammingError), + ({ 3: 2 }, TypeError), + ({ self: 2 }, TypeError), ] for parameter_sequence, exception in failing_cases: self.assertRaises(exception, cur.callproc, procname, parameter_sequence) From f53de458c92a835c58af536e62bf0fdfaed30cef Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 5 Jun 2014 02:32:53 +0200 Subject: [PATCH 11/15] Added test with objects without length as callproc param --- tests/test_cursor.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_cursor.py b/tests/test_cursor.py index fc389e1a..1ebe9d05 100755 --- a/tests/test_cursor.py +++ b/tests/test_cursor.py @@ -465,6 +465,11 @@ class CursorTests(ConnectingTestCase): self.assertRaises(exception, cur.callproc, procname, parameter_sequence) self.conn.rollback() + def test_callproc_badparam(self): + cur = self.conn.cursor() + self.assertRaises(TypeError, cur.callproc, 'lower', 42) + + def test_suite(): return unittest.TestLoader().loadTestsFromName(__name__) From ad89362b5057ec5f6b8868e30056270f10370625 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 5 Jun 2014 12:44:04 +0200 Subject: [PATCH 12/15] More straightforward param refcount handling in callproc --- psycopg/cursor_type.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 8282e661..8ab2d3a6 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1013,6 +1013,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) char *sql = NULL; Py_ssize_t procname_len, i, nparameters = 0, sl = 0; PyObject *parameters = Py_None; + PyObject *pvals = NULL; PyObject *operation = NULL; PyObject *res = NULL; @@ -1050,9 +1051,8 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) /* a Dict is complicated; the parameter names go into the query */ if (using_dict) { #if PG_VERSION_HEX >= 0x090000 - if (!(pnames = PyDict_Keys(parameters))) { - goto exit; - } + if (!(pnames = PyDict_Keys(parameters))) { goto exit; } + if (!(pvals = PyDict_Values(parameters))) { goto exit; } sl = procname_len + 17 + nparameters * 5 - (nparameters ? 1 : 0); @@ -1097,9 +1097,6 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) sql[sl-2] = ')'; sql[sl-1] = '\0'; - if (!(parameters = PyDict_Values(parameters))) { - goto exit; - } #else PyErr_SetString(PyExc_NotImplementedError, "named parameters require psycopg2 compiled against libpq 9.0+"); @@ -1109,6 +1106,9 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) /* a list (or None, or empty data structure) is a little bit simpler */ else { + Py_INCREF(parameters); + pvals = parameters; + sl = procname_len + 17 + nparameters * 3 - (nparameters ? 1 : 0); sql = (char*)PyMem_Malloc(sl); @@ -1129,11 +1129,9 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) goto exit; } - if (0 <= _psyco_curs_execute(self, operation, parameters, - self->conn->async, 0)) { - if (using_dict) { - Py_DECREF(parameters); - } + if (0 <= _psyco_curs_execute( + self, operation, pvals, self->conn->async, 0)) { + /* return None from this until it's DBAPI compliant... */ res = Py_None; } @@ -1152,6 +1150,7 @@ exit: Py_XDECREF(pnames); #endif Py_XDECREF(operation); + Py_XDECREF(pvals); PyMem_Free((void*)sql); return res; } From 5df70ff0bff429d5ca0acc279ea4d8cb94eb0a90 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 5 Jun 2014 12:45:39 +0200 Subject: [PATCH 13/15] Fixed callproc return value refcount Temporary anyway: I want to go back returning a list (or dict). --- psycopg/cursor_type.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 8ab2d3a6..05241de8 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1132,8 +1132,9 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) if (0 <= _psyco_curs_execute( self, operation, pvals, self->conn->async, 0)) { - /* return None from this until it's DBAPI compliant... */ - res = Py_None; + /* return None from this until it's DBAPI compliant... */ + Py_INCREF(Py_None); + res = Py_None; } exit: From 2a380eed0c28a583e4af187caece087f4854f097 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 5 Jun 2014 13:17:51 +0200 Subject: [PATCH 14/15] Set an exception in case of PQescapeIdentifier error Ifdeffed surface reduced. --- psycopg/cursor_type.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 05241de8..2d3c9412 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1006,6 +1006,34 @@ exit: #define psyco_curs_callproc_doc \ "callproc(procname, parameters=None) -- Execute stored procedure." +/* Call PQescapeIdentifier. + * + * In case of error set a Python exception. + * + * TODO: this function can become more generic and go into utils + */ +static char * +_escape_identifier(PGconn *pgconn, const char *str, size_t length) +{ + char *rv = NULL; + +#if PG_VERSION_HEX >= 0x090000 + rv = PQescapeIdentifier(pgconn, str, length); + if (!rv) { + char *msg; + if (!(msg = PQerrorMessage(pgconn))) { + msg = "no message provided"; + } + PyErr_Format(InterfaceError, "failed to escape identifier: %s", msg); + } +#else + PyErr_Format(PyExc_NotImplementedError, + "named parameters require psycopg2 compiled against libpq 9.0+"); +#endif + + return rv; +} + static PyObject * psyco_curs_callproc(cursorObject *self, PyObject *args) { @@ -1013,17 +1041,15 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) char *sql = NULL; Py_ssize_t procname_len, i, nparameters = 0, sl = 0; PyObject *parameters = Py_None; - PyObject *pvals = NULL; PyObject *operation = NULL; PyObject *res = NULL; int using_dict; -#if PG_VERSION_HEX >= 0x090000 PyObject *pname = NULL; PyObject *pnames = NULL; + PyObject *pvals = NULL; char *cpname = NULL; char **scpnames = NULL; -#endif if (!PyArg_ParseTuple(args, "s#|O", &procname, &procname_len, ¶meters)) { @@ -1050,7 +1076,6 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) /* a Dict is complicated; the parameter names go into the query */ if (using_dict) { -#if PG_VERSION_HEX >= 0x090000 if (!(pnames = PyDict_Keys(parameters))) { goto exit; } if (!(pvals = PyDict_Values(parameters))) { goto exit; } @@ -1074,7 +1099,7 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) if (!(pname = psycopg_ensure_bytes(pname))) { goto exit; } if (!(cpname = Bytes_AsString(pname))) { goto exit; } - if (!(scpnames[i] = PQescapeIdentifier( + if (!(scpnames[i] = _escape_identifier( self->conn->pgconn, cpname, strlen(cpname)))) { goto exit; } @@ -1096,12 +1121,6 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) } sql[sl-2] = ')'; sql[sl-1] = '\0'; - -#else - PyErr_SetString(PyExc_NotImplementedError, - "named parameters require psycopg2 compiled against libpq 9.0+"); - goto exit; -#endif } /* a list (or None, or empty data structure) is a little bit simpler */ @@ -1138,7 +1157,6 @@ psyco_curs_callproc(cursorObject *self, PyObject *args) } exit: -#if PG_VERSION_HEX >= 0x090000 if (scpnames != NULL) { for (i = 0; i < nparameters; i++) { if (scpnames[i] != NULL) { @@ -1149,7 +1167,6 @@ exit: PyMem_Del(scpnames); Py_XDECREF(pname); Py_XDECREF(pnames); -#endif Py_XDECREF(operation); Py_XDECREF(pvals); PyMem_Free((void*)sql); From 34d3ac428c81b2dce93d3202af978e977de4b099 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 5 Jun 2014 13:41:45 +0200 Subject: [PATCH 15/15] Correctly handle an empty error message from PQescapeIdentifier --- psycopg/cursor_type.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/psycopg/cursor_type.c b/psycopg/cursor_type.c index 2d3c9412..aac22a57 100644 --- a/psycopg/cursor_type.c +++ b/psycopg/cursor_type.c @@ -1021,7 +1021,8 @@ _escape_identifier(PGconn *pgconn, const char *str, size_t length) rv = PQescapeIdentifier(pgconn, str, length); if (!rv) { char *msg; - if (!(msg = PQerrorMessage(pgconn))) { + msg = PQerrorMessage(pgconn); + if (!msg || !msg[0]) { msg = "no message provided"; } PyErr_Format(InterfaceError, "failed to escape identifier: %s", msg);