FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-152817: Prevent deletion of sqlite3 `cursor.row_factory` attr, missed from: gh-149738 by stestagg · Pull Request #152818 · python/cpython · GitHub

Repository navigation

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

serhiy-storchaka merged 7 commits into
python:mainfrom
stestagg:main

Conversation

stestagg commented Jul 1, 2026 •
edited by bedevere-app Bot
Loading

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 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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 2, 2026 •
edited
Loading

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!

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 commented Jul 2, 2026 •
edited
Loading

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 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 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

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The same.

serhiy-storchaka enabled auto-merge (squash) August 21, 2026 07:28
serhiy-storchaka merged commit 8e96dd6 into python:main Aug 21, 2026
54 checks passed

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.
🐍🍒⛏🤖

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

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

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 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 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 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 Bot removed needs backport to 3.14 bugs and security fixes needs backport to 3.13 only 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>

Copy link
Copy Markdown
Contributor Author

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

serhiy-storchaka added a commit that referenced this pull request Oct 9, 2026
…GH-152818) (GH-156160)

(cherry picked from commit 8e96dd6)

Co-authored-by: Steve Stagg <stestagg@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL