Skip to content

bpo-45126: Harden sqlite3 connection initialisation #28227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Nov 16, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Sep 7, 2021

  • make sure we always start __init__ with a known connection state
  • make sure we always leave the connection in a consistent state
  • group member initialisation

Resolves #89289

https://bugs.python.org/issue45126

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Sep 7, 2021

cc. @encukounote this is still WIP

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Sep 8, 2021

I've split this PR up in two:

Merging at least the ref. leak PR first will greatly simplify this PR. This is already done ✓

Postponing cleanup to start of Python 3.13 dev.

@erlend-aasland erlend-aasland deleted the sqlite-cleanup-init branch September 8, 2021 10:40
@erlend-aasland erlend-aasland restored the sqlite-cleanup-init branch September 10, 2021 04:28
@erlend-aasland erlend-aasland changed the title bpo-45126: harden sqlite3 connection initialisation bpo-45126: Harden sqlite3 connection initialisation Sep 13, 2021
@erlend-aasland erlend-aasland marked this pull request as ready for review September 13, 2021 08:16
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@erlend-aasland erlend-aasland marked this pull request as draft October 18, 2021 12:41
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 18, 2021

I'm marking this as a draft until GH-29053 is merged.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Oct 19, 2021
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 15, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 257aba7 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 15, 2021
@erlend-aasland
Copy link
Contributor Author

Buildbot status:

  • AMD64 Arch Linux Asan Debug fails at test_gdb. It has been doing so for some time, so I recon it's unrelated.
  • AMD64 Windows10, AMD64 Windows10 Pro, Windows Non-Debug, ARM64 Windows fails at test_importlib; seems unrelated.

@erlend-aasland erlend-aasland marked this pull request as ready for review November 15, 2021 19:37
@erlend-aasland
Copy link
Contributor Author

@encukou, what do you think of this?

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Should connection_close set initialized = 0?

I don't think it's possible to think about (and test) all reinitialization edge cases, but this PR is an improvement.

@erlend-aasland
Copy link
Contributor Author

Should connection_close set initialized = 0?

That would need further changes to close(), since it already checks that member:

pysqlite_connection_close_impl(pysqlite_Connection *self)
/*[clinic end generated code: output=a546a0da212c9b97 input=3d58064bbffaa3d3]*/
{
if (!pysqlite_check_thread(self)) {
return NULL;
}
if (!self->initialized) {
PyTypeObject *tp = Py_TYPE(self);
pysqlite_state *state = pysqlite_get_state_by_type(tp);
PyErr_SetString(state->ProgrammingError,
"Base Connection.__init__ not called.");
return NULL;
}
Py_CLEAR(self->statement_cache);
connection_close(self);
Py_RETURN_NONE;
}

As a further improvement, we could look into just using the database pointer for verifying if a connection object is usable or not. That is, get rid of self->initialized and just use self->db. Note: I haven't tested this; it's just an idea.

I don't think it's possible to think about (and test) all reinitialization edge cases, but this PR is an improvement.

I agree with all of that.

@encukou
Copy link
Member

encukou commented Nov 16, 2021

As a further improvement, we could look into just using the database pointer for verifying if a connection object is usable or not. That is, get rid of self->initialized and just use self->db. Note: I haven't tested this; it's just an idea.

That sounds like a great idea, actually – less interdependent state to get out of sync!

@encukou encukou merged commit 9d6215a into python:main Nov 16, 2021
@erlend-aasland erlend-aasland deleted the sqlite-cleanup-init branch November 16, 2021 17:15
@erlend-aasland
Copy link
Contributor Author

Thanks for reviewing and merging, Petr!

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.

[sqlite3] cleanup and harden Connection.__init__
4 participants