bpo-31764: Prevent a crash in sqlite3.Cursor.close() in case the Cursor object is uninitialized #3958
Conversation
|
I wonder whether we should add However, i don't know how to produce code that would create a partially initialized |
Modules/_sqlite/cursor.c
Outdated
| @@ -890,6 +890,11 @@ PyObject* pysqlite_noop(pysqlite_Connection* self, PyObject* args) | |||
|
|
|||
| PyObject* pysqlite_cursor_close(pysqlite_Cursor* self, PyObject* args) | |||
| { | |||
| if (self->connection == NULL) { | |||
There was a problem hiding this comment.
Style nit: I think !foo is more common than foo == NULL in sqlite3 codebase. It would be nice to keep styling consistent within the module.
| @@ -0,0 +1,2 @@ | |||
| Prevent a crash in `sqlite3.Cursor.close()` in case the `Cursor` object is | |||
There was a problem hiding this comment.
Please use double backticks (``). I tried to explain why single backticks shouldn't be used at #3925 (comment)
There was a problem hiding this comment.
I am not familiar with Sphinx, but i trust you are :)
So maybe https://devguide.python.org/committing/#what-s-new-and-news-entries should also recommend double backticks?
There was a problem hiding this comment.
Yeah, I wasn't aware of that example in the devguide. I will update it, thanks!
Lib/sqlite3/test/regression.py
Outdated
| @@ -188,6 +188,7 @@ def __init__(self, con): | |||
| cur = Cursor(con) | |||
| with self.assertRaises(sqlite.ProgrammingError): | |||
| cur.execute("select 4+5").fetchall() | |||
| self.assertRaises(sqlite.ProgrammingError, cur.close) | |||
There was a problem hiding this comment.
ProgrammingError is raised by a lot of things so I wonder if we should add a test for the exception message to make it more future-proof.
There was a problem hiding this comment.
All right. Should i also change the assertRaises() that was already in CheckCursorConstructorCallCheck()?
There was a problem hiding this comment.
I'd say let's do it only for new additions to the test.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@berkerpeksag What do you think about #3958 (comment)? |
I agree with you. There is no need to add additional code for hypothetical use cases. We can change it later if we find a way to create a partially initialized Cursor object. |
vstinner
dismissed
berkerpeksag’s stale review
The author took comments in account.
…or object is uninitialized (pythonGH-3958) (cherry picked from commit edb13ae)
|
GH-4303 is a backport of this pull request to the 3.6 branch. |
…or object is uninitialized (python#3958)
In addition, add a test to
test_sqliteto make sure that the crash is no more.https://bugs.python.org/issue31764