From fc662ac332443a316a120fa5287c235dc4f8739b Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 10 Dec 2018 16:06:08 +0200 Subject: [PATCH] bpo-32788: Better error handling in sqlite3. (GH-3723) Propagate unexpected errors (like MemoryError and KeyboardInterrupt) to user. --- Lib/sqlite3/test/types.py | 28 +++- .../2018-07-23-12-20-02.bpo-32788.R2jSiK.rst | 3 + Modules/_sqlite/cache.c | 8 +- Modules/_sqlite/connection.c | 8 +- Modules/_sqlite/cursor.c | 125 ++++++++---------- Modules/_sqlite/microprotocols.c | 70 ++++++---- Modules/_sqlite/statement.c | 34 ++--- 7 files changed, 154 insertions(+), 122 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-07-23-12-20-02.bpo-32788.R2jSiK.rst diff --git a/Lib/sqlite3/test/types.py b/Lib/sqlite3/test/types.py index 6bc8d71def0..19ecd07500f 100644 --- a/Lib/sqlite3/test/types.py +++ b/Lib/sqlite3/test/types.py @@ -102,10 +102,16 @@ class DeclTypesTests(unittest.TestCase): def __str__(self): return "<%s>" % self.val + class BadConform: + def __init__(self, exc): + self.exc = exc + def __conform__(self, protocol): + raise self.exc + def setUp(self): self.con = sqlite.connect(":memory:", detect_types=sqlite.PARSE_DECLTYPES) self.cur = self.con.cursor() - self.cur.execute("create table test(i int, s str, f float, b bool, u unicode, foo foo, bin blob, n1 number, n2 number(5))") + self.cur.execute("create table test(i int, s str, f float, b bool, u unicode, foo foo, bin blob, n1 number, n2 number(5), bad bad)") # override float, make them always return the same number sqlite.converters["FLOAT"] = lambda x: 47.2 @@ -113,6 +119,7 @@ class DeclTypesTests(unittest.TestCase): # and implement two custom ones sqlite.converters["BOOL"] = lambda x: bool(int(x)) sqlite.converters["FOO"] = DeclTypesTests.Foo + sqlite.converters["BAD"] = DeclTypesTests.BadConform sqlite.converters["WRONG"] = lambda x: "WRONG" sqlite.converters["NUMBER"] = float @@ -120,6 +127,8 @@ class DeclTypesTests(unittest.TestCase): del sqlite.converters["FLOAT"] del sqlite.converters["BOOL"] del sqlite.converters["FOO"] + del sqlite.converters["BAD"] + del sqlite.converters["WRONG"] del sqlite.converters["NUMBER"] self.cur.close() self.con.close() @@ -159,13 +168,13 @@ class DeclTypesTests(unittest.TestCase): self.cur.execute("insert into test(b) values (?)", (False,)) self.cur.execute("select b from test") row = self.cur.fetchone() - self.assertEqual(row[0], False) + self.assertIs(row[0], False) self.cur.execute("delete from test") self.cur.execute("insert into test(b) values (?)", (True,)) self.cur.execute("select b from test") row = self.cur.fetchone() - self.assertEqual(row[0], True) + self.assertIs(row[0], True) def CheckUnicode(self): # default @@ -182,6 +191,19 @@ class DeclTypesTests(unittest.TestCase): row = self.cur.fetchone() self.assertEqual(row[0], val) + def CheckErrorInConform(self): + val = DeclTypesTests.BadConform(TypeError) + with self.assertRaises(sqlite.InterfaceError): + self.cur.execute("insert into test(bad) values (?)", (val,)) + with self.assertRaises(sqlite.InterfaceError): + self.cur.execute("insert into test(bad) values (:val)", {"val": val}) + + val = DeclTypesTests.BadConform(KeyboardInterrupt) + with self.assertRaises(KeyboardInterrupt): + self.cur.execute("insert into test(bad) values (?)", (val,)) + with self.assertRaises(KeyboardInterrupt): + self.cur.execute("insert into test(bad) values (:val)", {"val": val}) + def CheckUnsupportedSeq(self): class Bar: pass val = Bar() diff --git a/Misc/NEWS.d/next/Library/2018-07-23-12-20-02.bpo-32788.R2jSiK.rst b/Misc/NEWS.d/next/Library/2018-07-23-12-20-02.bpo-32788.R2jSiK.rst new file mode 100644 index 00000000000..1f3ae88b706 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-07-23-12-20-02.bpo-32788.R2jSiK.rst @@ -0,0 +1,3 @@ +Errors other than :exc:`TypeError` raised in methods ``__adapt__()`` and +``__conform__()`` in the :mod:`sqlite3` module are now propagated to the +user. diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c index 72b1f2c5614..4270473ae99 100644 --- a/Modules/_sqlite/cache.c +++ b/Modules/_sqlite/cache.c @@ -119,7 +119,7 @@ PyObject* pysqlite_cache_get(pysqlite_Cache* self, PyObject* args) pysqlite_Node* ptr; PyObject* data; - node = (pysqlite_Node*)PyDict_GetItem(self->mapping, key); + node = (pysqlite_Node*)PyDict_GetItemWithError(self->mapping, key); if (node) { /* an entry for this key already exists in the cache */ @@ -157,7 +157,11 @@ PyObject* pysqlite_cache_get(pysqlite_Cache* self, PyObject* args) } ptr->prev = node; } - } else { + } + else if (PyErr_Occurred()) { + return NULL; + } + else { /* There is no entry for this key in the cache, yet. We'll insert a new * entry in the cache, and make space if necessary by throwing the * least used item out of the cache. */ diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index fe0d03bb2b0..aab4b1afd22 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -1432,6 +1432,7 @@ finally: static PyObject * pysqlite_connection_iterdump(pysqlite_Connection* self, PyObject* args) { + _Py_IDENTIFIER(_iterdump); PyObject* retval = NULL; PyObject* module = NULL; PyObject* module_dict; @@ -1451,9 +1452,12 @@ pysqlite_connection_iterdump(pysqlite_Connection* self, PyObject* args) goto finally; } - pyfn_iterdump = PyDict_GetItemString(module_dict, "_iterdump"); + pyfn_iterdump = _PyDict_GetItemIdWithError(module_dict, &PyId__iterdump); if (!pyfn_iterdump) { - PyErr_SetString(pysqlite_OperationalError, "Failed to obtain _iterdump() reference"); + if (!PyErr_Occurred()) { + PyErr_SetString(pysqlite_OperationalError, + "Failed to obtain _iterdump() reference"); + } goto finally; } diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 8a46e5cf2eb..2bc19311a81 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -94,40 +94,47 @@ static void pysqlite_cursor_dealloc(pysqlite_Cursor* self) Py_TYPE(self)->tp_free((PyObject*)self); } -PyObject* _pysqlite_get_converter(PyObject* key) +static PyObject * +_pysqlite_get_converter(const char *keystr, Py_ssize_t keylen) { - PyObject* upcase_key; - PyObject* retval; + PyObject *key; + PyObject *upcase_key; + PyObject *retval; _Py_IDENTIFIER(upper); + key = PyUnicode_FromStringAndSize(keystr, keylen); + if (!key) { + return NULL; + } upcase_key = _PyObject_CallMethodId(key, &PyId_upper, NULL); + Py_DECREF(key); if (!upcase_key) { return NULL; } - retval = PyDict_GetItem(_pysqlite_converters, upcase_key); + retval = PyDict_GetItemWithError(_pysqlite_converters, upcase_key); Py_DECREF(upcase_key); return retval; } -int pysqlite_build_row_cast_map(pysqlite_Cursor* self) +static int +pysqlite_build_row_cast_map(pysqlite_Cursor* self) { int i; - const char* type_start = (const char*)-1; const char* pos; - const char* colname; const char* decltype; - PyObject* py_decltype; PyObject* converter; - PyObject* key; if (!self->connection->detect_types) { return 0; } Py_XSETREF(self->row_cast_map, PyList_New(0)); + if (!self->row_cast_map) { + return -1; + } for (i = 0; i < sqlite3_column_count(self->statement->st); i++) { converter = NULL; @@ -135,20 +142,17 @@ int pysqlite_build_row_cast_map(pysqlite_Cursor* self) if (self->connection->detect_types & PARSE_COLNAMES) { colname = sqlite3_column_name(self->statement->st, i); if (colname) { + const char *type_start = NULL; for (pos = colname; *pos != 0; pos++) { if (*pos == '[') { type_start = pos + 1; - } else if (*pos == ']' && type_start != (const char*)-1) { - key = PyUnicode_FromStringAndSize(type_start, pos - type_start); - if (!key) { - /* creating a string failed, but it is too complicated - * to propagate the error here, we just assume there is - * no converter and proceed */ - break; + } + else if (*pos == ']' && type_start != NULL) { + converter = _pysqlite_get_converter(type_start, pos - type_start); + if (!converter && PyErr_Occurred()) { + Py_CLEAR(self->row_cast_map); + return -1; } - - converter = _pysqlite_get_converter(key); - Py_DECREF(key); break; } } @@ -164,16 +168,14 @@ int pysqlite_build_row_cast_map(pysqlite_Cursor* self) * 'NUMBER(10)' to be treated as 'NUMBER', for example. * In other words, it will work as people expect it to work.*/ if (*pos == ' ' || *pos == '(' || *pos == 0) { - py_decltype = PyUnicode_FromStringAndSize(decltype, pos - decltype); - if (!py_decltype) { + converter = _pysqlite_get_converter(decltype, pos - decltype); + if (!converter && PyErr_Occurred()) { + Py_CLEAR(self->row_cast_map); return -1; } break; } } - - converter = _pysqlite_get_converter(py_decltype); - Py_DECREF(py_decltype); } } @@ -182,11 +184,7 @@ int pysqlite_build_row_cast_map(pysqlite_Cursor* self) } if (PyList_Append(self->row_cast_map, converter) != 0) { - if (converter != Py_None) { - Py_DECREF(converter); - } Py_CLEAR(self->row_cast_map); - return -1; } } @@ -194,7 +192,8 @@ int pysqlite_build_row_cast_map(pysqlite_Cursor* self) return 0; } -PyObject* _pysqlite_build_column_name(const char* colname) +static PyObject * +_pysqlite_build_column_name(const char* colname) { const char* pos; @@ -218,7 +217,8 @@ PyObject* _pysqlite_build_column_name(const char* colname) * Precondidition: * - sqlite3_step() has been called before and it returned SQLITE_ROW. */ -PyObject* _pysqlite_fetch_one_row(pysqlite_Cursor* self) +static PyObject * +_pysqlite_fetch_one_row(pysqlite_Cursor* self) { int i, numcols; PyObject* row; @@ -227,12 +227,10 @@ PyObject* _pysqlite_fetch_one_row(pysqlite_Cursor* self) PyObject* converter; PyObject* converted; Py_ssize_t nbytes; - PyObject* buffer; const char* val_str; char buf[200]; const char* colname; - PyObject* buf_bytes; - PyObject* error_obj; + PyObject* error_msg; if (self->reset) { PyErr_SetString(pysqlite_InterfaceError, errmsg_fetch_across_rollback); @@ -248,13 +246,13 @@ PyObject* _pysqlite_fetch_one_row(pysqlite_Cursor* self) return NULL; for (i = 0; i < numcols; i++) { - if (self->connection->detect_types) { - assert(self->row_cast_map != NULL); - converter = PyList_GetItem(self->row_cast_map, i); - if (!converter) { - converter = Py_None; - } - } else { + if (self->connection->detect_types + && self->row_cast_map != NULL + && i < PyList_GET_SIZE(self->row_cast_map)) + { + converter = PyList_GET_ITEM(self->row_cast_map, i); + } + else { converter = Py_None; } @@ -270,8 +268,6 @@ PyObject* _pysqlite_fetch_one_row(pysqlite_Cursor* self) goto error; converted = PyObject_CallFunction(converter, "O", item); Py_DECREF(item); - if (!converted) - break; } } else { Py_BEGIN_ALLOW_THREADS @@ -289,7 +285,7 @@ PyObject* _pysqlite_fetch_one_row(pysqlite_Cursor* self) nbytes = sqlite3_column_bytes(self->statement->st, i); if (self->connection->text_factory == (PyObject*)&PyUnicode_Type) { converted = PyUnicode_FromStringAndSize(val_str, nbytes); - if (!converted) { + if (!converted && PyErr_ExceptionMatches(PyExc_UnicodeDecodeError)) { PyErr_Clear(); colname = sqlite3_column_name(self->statement->st, i); if (!colname) { @@ -297,18 +293,12 @@ PyObject* _pysqlite_fetch_one_row(pysqlite_Cursor* self) } PyOS_snprintf(buf, sizeof(buf) - 1, "Could not decode to UTF-8 column '%s' with text '%s'", colname , val_str); - buf_bytes = PyByteArray_FromStringAndSize(buf, strlen(buf)); - if (!buf_bytes) { + error_msg = PyUnicode_Decode(buf, strlen(buf), "ascii", "replace"); + if (!error_msg) { PyErr_SetString(pysqlite_OperationalError, "Could not decode to UTF-8"); } else { - error_obj = PyUnicode_FromEncodedObject(buf_bytes, "ascii", "replace"); - if (!error_obj) { - PyErr_SetString(pysqlite_OperationalError, "Could not decode to UTF-8"); - } else { - PyErr_SetObject(pysqlite_OperationalError, error_obj); - Py_DECREF(error_obj); - } - Py_DECREF(buf_bytes); + PyErr_SetObject(pysqlite_OperationalError, error_msg); + Py_DECREF(error_msg); } } } else if (self->connection->text_factory == (PyObject*)&PyBytes_Type) { @@ -321,20 +311,15 @@ PyObject* _pysqlite_fetch_one_row(pysqlite_Cursor* self) } else { /* coltype == SQLITE_BLOB */ nbytes = sqlite3_column_bytes(self->statement->st, i); - buffer = PyBytes_FromStringAndSize( + converted = PyBytes_FromStringAndSize( sqlite3_column_blob(self->statement->st, i), nbytes); - if (!buffer) - break; - converted = buffer; } } - if (converted) { - PyTuple_SetItem(row, i, converted); - } else { - Py_INCREF(Py_None); - PyTuple_SetItem(row, i, Py_None); + if (!converted) { + goto error; } + PyTuple_SetItem(row, i, converted); } if (PyErr_Occurred()) @@ -372,7 +357,8 @@ static int check_cursor(pysqlite_Cursor* cur) return pysqlite_check_thread(cur->connection) && pysqlite_check_connection(cur->connection); } -PyObject* _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* args) +static PyObject * +_pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* args) { PyObject* operation; PyObject* parameters_list = NULL; @@ -542,7 +528,7 @@ PyObject* _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* } if (pysqlite_build_row_cast_map(self) != 0) { - PyErr_SetString(pysqlite_OperationalError, "Error while building row_cast_map"); + _PyErr_FormatFromCause(pysqlite_OperationalError, "Error while building row_cast_map"); goto error; } @@ -631,7 +617,8 @@ PyObject* pysqlite_cursor_executemany(pysqlite_Cursor* self, PyObject* args) return _pysqlite_query_execute(self, 1, args); } -PyObject* pysqlite_cursor_executescript(pysqlite_Cursor* self, PyObject* args) +static PyObject * +pysqlite_cursor_executescript(pysqlite_Cursor* self, PyObject* args) { PyObject* script_obj; PyObject* script_str = NULL; @@ -718,12 +705,6 @@ error: } } -PyObject* pysqlite_cursor_getiter(pysqlite_Cursor *self) -{ - Py_INCREF(self); - return (PyObject*)self; -} - PyObject* pysqlite_cursor_iternext(pysqlite_Cursor *self) { PyObject* next_row_tuple; @@ -961,7 +942,7 @@ PyTypeObject pysqlite_CursorType = { 0, /* tp_clear */ 0, /* tp_richcompare */ offsetof(pysqlite_Cursor, in_weakreflist), /* tp_weaklistoffset */ - (getiterfunc)pysqlite_cursor_getiter, /* tp_iter */ + PyObject_SelfIter, /* tp_iter */ (iternextfunc)pysqlite_cursor_iternext, /* tp_iternext */ cursor_methods, /* tp_methods */ cursor_members, /* tp_members */ diff --git a/Modules/_sqlite/microprotocols.c b/Modules/_sqlite/microprotocols.c index 3d01872322c..c23b09f56b5 100644 --- a/Modules/_sqlite/microprotocols.c +++ b/Modules/_sqlite/microprotocols.c @@ -75,7 +75,9 @@ pysqlite_microprotocols_add(PyTypeObject *type, PyObject *proto, PyObject *cast) PyObject * pysqlite_microprotocols_adapt(PyObject *obj, PyObject *proto, PyObject *alt) { - PyObject *adapter, *key; + _Py_IDENTIFIER(__adapt__); + _Py_IDENTIFIER(__conform__); + PyObject *adapter, *key, *adapted; /* we don't check for exact type conformance as specified in PEP 246 because the pysqlite_PrepareProtocolType type is abstract and there is no @@ -86,48 +88,60 @@ pysqlite_microprotocols_adapt(PyObject *obj, PyObject *proto, PyObject *alt) if (!key) { return NULL; } - adapter = PyDict_GetItem(psyco_adapters, key); + adapter = PyDict_GetItemWithError(psyco_adapters, key); Py_DECREF(key); if (adapter) { - PyObject *adapted = PyObject_CallFunctionObjArgs(adapter, obj, NULL); + Py_INCREF(adapter); + adapted = PyObject_CallFunctionObjArgs(adapter, obj, NULL); + Py_DECREF(adapter); return adapted; } + if (PyErr_Occurred()) { + return NULL; + } - /* try to have the protocol adapt this object*/ - if (PyObject_HasAttrString(proto, "__adapt__")) { - _Py_IDENTIFIER(__adapt__); - PyObject *adapted = _PyObject_CallMethodId(proto, &PyId___adapt__, "O", obj); + /* try to have the protocol adapt this object */ + if (_PyObject_LookupAttrId(proto, &PyId___adapt__, &adapter) < 0) { + return NULL; + } + if (adapter) { + adapted = PyObject_CallFunctionObjArgs(adapter, obj, NULL); + Py_DECREF(adapter); - if (adapted) { - if (adapted != Py_None) { - return adapted; - } else { - Py_DECREF(adapted); - } + if (adapted == Py_None) { + Py_DECREF(adapted); + } + else if (adapted || !PyErr_ExceptionMatches(PyExc_TypeError)) { + return adapted; + } + else { + PyErr_Clear(); } - - if (PyErr_Occurred() && !PyErr_ExceptionMatches(PyExc_TypeError)) - return NULL; } /* and finally try to have the object adapt itself */ - if (PyObject_HasAttrString(obj, "__conform__")) { - _Py_IDENTIFIER(__conform__); - PyObject *adapted = _PyObject_CallMethodId(obj, &PyId___conform__,"O", proto); + if (_PyObject_LookupAttrId(obj, &PyId___conform__, &adapter) < 0) { + return NULL; + } + if (adapter) { + adapted = PyObject_CallFunctionObjArgs(adapter, proto, NULL); + Py_DECREF(adapter); - if (adapted) { - if (adapted != Py_None) { - return adapted; - } else { - Py_DECREF(adapted); - } + if (adapted == Py_None) { + Py_DECREF(adapted); } - - if (PyErr_Occurred() && !PyErr_ExceptionMatches(PyExc_TypeError)) { - return NULL; + else if (adapted || !PyErr_ExceptionMatches(PyExc_TypeError)) { + return adapted; + } + else { + PyErr_Clear(); } } + if (alt) { + Py_INCREF(alt); + return alt; + } /* else set the right exception and return NULL */ PyErr_SetString(pysqlite_ProgrammingError, "can't adapt"); return NULL; diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 78033d8efca..575ac69d903 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -250,12 +250,10 @@ void pysqlite_statement_bind_parameters(pysqlite_Statement* self, PyObject* para if (!_need_adapt(current_param)) { adapted = current_param; } else { - adapted = pysqlite_microprotocols_adapt(current_param, (PyObject*)&pysqlite_PrepareProtocolType, NULL); - if (adapted) { - Py_DECREF(current_param); - } else { - PyErr_Clear(); - adapted = current_param; + adapted = pysqlite_microprotocols_adapt(current_param, (PyObject*)&pysqlite_PrepareProtocolType, current_param); + Py_DECREF(current_param); + if (!adapted) { + return; } } @@ -272,6 +270,7 @@ void pysqlite_statement_bind_parameters(pysqlite_Statement* self, PyObject* para } else if (PyDict_Check(parameters)) { /* parameters passed as dictionary */ for (i = 1; i <= num_params_needed; i++) { + PyObject *binding_name_obj; Py_BEGIN_ALLOW_THREADS binding_name = sqlite3_bind_parameter_name(self->st, i); Py_END_ALLOW_THREADS @@ -281,26 +280,31 @@ void pysqlite_statement_bind_parameters(pysqlite_Statement* self, PyObject* para } binding_name++; /* skip first char (the colon) */ + binding_name_obj = PyUnicode_FromString(binding_name); + if (!binding_name_obj) { + return; + } if (PyDict_CheckExact(parameters)) { - current_param = PyDict_GetItemString(parameters, binding_name); + current_param = PyDict_GetItemWithError(parameters, binding_name_obj); Py_XINCREF(current_param); } else { - current_param = PyMapping_GetItemString(parameters, binding_name); + current_param = PyObject_GetItem(parameters, binding_name_obj); } + Py_DECREF(binding_name_obj); if (!current_param) { - PyErr_Format(pysqlite_ProgrammingError, "You did not supply a value for binding %d.", i); + if (!PyErr_Occurred() || PyErr_ExceptionMatches(PyExc_LookupError)) { + PyErr_Format(pysqlite_ProgrammingError, "You did not supply a value for binding %d.", i); + } return; } if (!_need_adapt(current_param)) { adapted = current_param; } else { - adapted = pysqlite_microprotocols_adapt(current_param, (PyObject*)&pysqlite_PrepareProtocolType, NULL); - if (adapted) { - Py_DECREF(current_param); - } else { - PyErr_Clear(); - adapted = current_param; + adapted = pysqlite_microprotocols_adapt(current_param, (PyObject*)&pysqlite_PrepareProtocolType, current_param); + Py_DECREF(current_param); + if (!adapted) { + return; } }