Skip to content

gh-152817: Prevent deletion of sqlite3 cursor.row_factory attr, missed from: gh-149738#152818

Open
stestagg wants to merge 3 commits into
python:mainfrom
stestagg:main
Open

gh-152817: Prevent deletion of sqlite3 cursor.row_factory attr, missed from: gh-149738#152818
stestagg wants to merge 3 commits into
python:mainfrom
stestagg:main

Conversation

@stestagg

@stestagg stestagg commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

gh-149738 prevents deletion of row_factory and text_factory attributes on a sqlite3 connection, this PR applies that fix to cursor objects too, to avoid a similar segfault:

import sqlite3

cur = sqlite3.connect(":memory:").cursor()
del cur.row_factory
cur.execute("select 1").fetchone()

I've piggy-backed off the existing docs change and blurb as that seems to cover this already

@stestagg

stestagg commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@sepehr-rs - You were the author of the original, are you able to take a look? Thanks in advance!

@sepehr-rs sepehr-rs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @stestagg, thank you for reaching out to me on this!
I think your change is good, but I believe you should also add a versionchanged:: next note under the Cursor.row_factory attribute docs, mirroring what was added for Connection.row_factory/text_factory. Besides that, +1 from me.

Comment thread Modules/_sqlite/cursor.c
@stestagg

stestagg commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Hi @stestagg, thank you for reaching out to me on this! I think your change is good, but I believe you should also add a versionchanged:: next note under the Cursor.row_factory attribute docs, mirroring what was added for Connection.row_factory/text_factory. Besides that, +1 from me.

As I understand it, gh-149738 already altered the doc for the Cursor.row_factory attr (as well as for the Connection.row_factory), just not the code:

.. versionchanged:: next
Deleting the ``row_factory`` attribute is no longer allowed.

It's likely I've misunderstood/missed something here!

@sepehr-rs

Copy link
Copy Markdown
Contributor

As I understand it, gh-149738 already altered the doc for the Cursor.row_factory attr (as well as for the Connection.row_factory), just not the code:

Oh, you're right. I must have forgotten about that. This is a real gap, thanks for checking this! :)

@stestagg

stestagg commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

There's a related issue that I uncovered above, if you create a Connection or Cursor without calling __init__ on it (why would you do this?) then row_factory/text_factory fields are never initialized, so then you get a segfault anyway.

To me, there's a few options:

  1. Make the users of row_factory/text_factory handle NULL like None, it's a cheap check, then having a NULL in this fields doesn't matter, we can remove the 'not allowed to del the attribute' code which makes things a bit simpler.
  2. Move the current field initialisation into tp_new (currently we don't have one), so that we never have an object in python space with row_factory=NULL, we'd have to keep the delattr guards, but that's fine, why would you delattr this anyway
  3. Just ignore the above issue, get this PR in, and raise a new issue to fix the new/init problem.

I'm not sure of the best approach tbh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants