gh-149738: Fix segmentation fault bug in sqllite3#149754
Conversation
picnixz
left a comment
There was a problem hiding this comment.
We need tests and check what happens on the created connection after deletion (check that there is still a row factory on the newly created cursor)
|
Thanks for your comment and review, @picnixz! P.S. The Read the Docs build is failing, but the logs indicate it’s due to an external issue unrelated to my changes. |
vstinner
left a comment
There was a problem hiding this comment.
I would prefer using connection_getset instead of connection_members for row_factory and text_factory to raise an exception if the developer tries to delete the attribute (set it to NULL). Add get/set on these attributes.
|
Hi @vstinner! Thank you for your kind review. Sorry for the delay, GitHub access has been unreliable here recently. |
picnixz
left a comment
There was a problem hiding this comment.
Plesse correct undefined behaviors by changing the getset signatures. There should be no cast to getter/setter.
| } | ||
|
|
||
| static int | ||
| connection_set_text_factory(pysqlite_Connection *self, PyObject *value, void *closure) |
There was a problem hiding this comment.
The signature is incorrect, it must be (PyObject*, PyObject*, void*) and you must cast self in the body.
|
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 |
| @@ -0,0 +1,2 @@ | |||
| :mod:`sqlite3`: Disallow removing ``row_factory`` and ``text_factory`` attributes | |||
There was a problem hiding this comment.
Pleas update the docs of salite3.rst as well to indicate this change using a versionchanged directive.
| } | ||
|
|
||
| static PyObject * | ||
| connection_get_row_factory(pysqlite_Connection *self, void *closure) |
There was a problem hiding this comment.
Those getsets must be using a critical section as well.
Fixes #149738.