Skip to content

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

Merged
serhiy-storchaka merged 7 commits into
python:mainfrom
stestagg:main
Aug 21, 2026
Merged

gh-152817: Prevent deletion of sqlite3 cursor.row_factory attr, missed from: gh-149738#152818
serhiy-storchaka merged 7 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.

@sobolevn

sobolevn commented Jul 3, 2026

Copy link
Copy Markdown
Member

I propose to split this into 2 PRs: one with the deletion, one with the assignement. Let's keep this PR focused on one thing :)

@stestagg

stestagg commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@sobolevn I propose to split this into 2 PRs: one with the deletion, one with the assignement. Let's keep this PR focused on one thing :)

fair point, reverted, I'll raise the other part separately

@stestagg

Copy link
Copy Markdown
Contributor Author

Hi! Is there anything I can do to progress this?

I'll do a rebase on main to check everything's ok, and try to clear the test failure (seems like they're unrelated)

prevent segfault and bring behaviour in line with docs
re-initialize in tp_init (because of tp_clear calls)
and also guard each use of row_factory and text_factory
with an explicit NULL check.

Keep the previous delattr guards, because who would be calling
__delattr__ on these anyway.

I'm imagining someone will suggest removing some of these changes,
but something something seek forgiveness something.
@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes labels Aug 20, 2026

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM besides few nitpicks.

Comment thread Modules/_sqlite/cursor.c Outdated
};

static PyObject *
cursor_get_row_factory(PyObject *op, void *closure)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Style nitpick: use Py_UNUSED(closure).

Comment thread Modules/_sqlite/cursor.c Outdated
static PyObject *
cursor_get_row_factory(PyObject *op, void *closure)
{
pysqlite_Cursor *self = (pysqlite_Cursor *)op;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use _pysqlite_Cursor_CAST().

Comment thread Modules/_sqlite/cursor.c Outdated
Comment on lines +1415 to +1417
cursor_set_row_factory(PyObject *op, PyObject *value, void *closure)
{
pysqlite_Cursor *self = (pysqlite_Cursor *)op;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The same.

Comment thread Lib/test/test_sqlite3/test_factory.py
@serhiy-storchaka
serhiy-storchaka enabled auto-merge (squash) August 21, 2026 07:28
@serhiy-storchaka
serhiy-storchaka merged commit 8e96dd6 into python:main Aug 21, 2026
54 checks passed
@miss-islington-app

Copy link
Copy Markdown

Thanks @stestagg for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15.
🐍🍒⛏🤖

@miss-islington-app

Copy link
Copy Markdown

Sorry, @stestagg and @serhiy-storchaka, I could not cleanly backport this to 3.15 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8e96dd6f25b53f0773d3f3f2a898eaa1501ee92e 3.15

@miss-islington-app

Copy link
Copy Markdown

Sorry, @stestagg and @serhiy-storchaka, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8e96dd6f25b53f0773d3f3f2a898eaa1501ee92e 3.14

@miss-islington-app

Copy link
Copy Markdown

Sorry, @stestagg and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8e96dd6f25b53f0773d3f3f2a898eaa1501ee92e 3.13

@bedevere-app

bedevere-app Bot commented Aug 21, 2026

Copy link
Copy Markdown

GH-156160 is a backport of this pull request to the 3.15 branch.

@bedevere-app

bedevere-app Bot commented Aug 21, 2026

Copy link
Copy Markdown

GH-156161 is a backport of this pull request to the 3.14 branch.

@bedevere-app

bedevere-app Bot commented Aug 21, 2026

Copy link
Copy Markdown

GH-156162 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed needs backport to 3.14 bugs and security fixes needs backport to 3.13 bugs and security fixes labels Aug 21, 2026
serhiy-storchaka added a commit that referenced this pull request Aug 21, 2026
…GH-152818) (GH-156162)

(cherry picked from commit 8e96dd6)

Co-authored-by: Steve Stagg <stestagg@gmail.com>
serhiy-storchaka added a commit that referenced this pull request Aug 21, 2026
…GH-152818) (GH-156161)

(cherry picked from commit 8e96dd6)

Co-authored-by: Steve Stagg <stestagg@gmail.com>
@stestagg

Copy link
Copy Markdown
Contributor Author

Thanks @serhiy-storchaka! I was planning on looking at the comments today, but you saved me :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants