Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sqlite3: some code paths ignore exceptions #108083

Closed
erlend-aasland opened this issue Aug 17, 2023 · 8 comments
Closed

sqlite3: some code paths ignore exceptions #108083

erlend-aasland opened this issue Aug 17, 2023 · 8 comments
Assignees
@erlend-aasland
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes topic-sqlite3 type-bug An unexpected behavior, bug, or error

Comments

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Aug 17, 2023 •
edited by bedevere-bot
Loading

Originally posted by @vstinner in #108015 (comment):

Ignoring the exception here is a bug.

connection_finalize() clears any exception with PyErr_SetRaisedException(), but pysqlite_connection_close_impl() must not ignore silently error, since here we are talking about a raised Python exception! The function must report if an exception was raised. Then the caller is free to ignore it or not.

I suggest to continue ignoring it in finalize, but then write a separated PR to log the "unraisable exception".

Linked PRs
@erlend-aasland erlend-aasland self-assigned this Aug 17, 2023
@erlend-aasland erlend-aasland added topic-sqlite3 type-bug An unexpected behavior, bug, or error 3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes labels Aug 17, 2023
@bedevere-bot bedevere-bot mentioned this issue Aug 17, 2023
Merged
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 17, 2023
@erlend-aasland
pythongh-108083: Don't ignore exceptions in sqlite3.Connection.__init…
0956795 
…__() and .close()

Always check the return value of connection_exec_stmt()
@serhiy-storchaka
Copy link
Member

I have not found a test for failing Connection.close(). Would it be possible to write one? Maybe in a separate issue.

erlend-aasland added a commit that referenced this issue Aug 18, 2023
@erlend-aasland @vstinner @serhiy-storchaka
gh-108083: Don't ignore exceptions in sqlite3.Connection.__init__() a…
fd19509 
…nd .close() (#108084)

- Add explanatory comments
- Add return value to connection_close() for propagating errors
- Always check the return value of connection_exec_stmt()
- Assert pre/post state in remove_callbacks()
- Don't log unraisable exceptions in case of interpreter shutdown
- Make sure we're not initialized if reinit fails
- Try to close the database even if ROLLBACK fails

Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot bedevere-bot mentioned this issue Aug 19, 2023
Merged
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 19, 2023
@erlend-aasland @vstinner @serhiy-storchaka
[3.12] pythongh-108083: Don't ignore exceptions in sqlite3.Connection…
488886c 
….__init__() and .close() (python#108084)

- Add explanatory comments
- Add return value to connection_close() for propagating errors
- Always check the return value of connection_exec_stmt()
- Assert pre/post state in remove_callbacks()
- Don't log unraisable exceptions in case of interpreter shutdown
- Make sure we're not initialized if reinit fails
- Try to close the database even if ROLLBACK fails

(cherry picked from commit fd19509)

Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@erlend-aasland
Copy link
Contributor Author

I have not found a test for failing Connection.close(). Would it be possible to write one? Maybe in a separate issue.

It should be possible to force the implicit ROLLBACK in .close() to fail using some kind of database lock trick. Perhaps we could use a similar trick as in test_ctx_mgr_rollback_if_commit_failed.

Yhg1s pushed a commit that referenced this issue Aug 19, 2023
@erlend-aasland @vstinner @serhiy-storchaka
[3.12] gh-108083: Don't ignore exceptions in sqlite3.Connection.__ini…
0c21298 
…t__() and .close() (#108084) (#108141)

- Add explanatory comments
- Add return value to connection_close() for propagating errors
- Always check the return value of connection_exec_stmt()
- Assert pre/post state in remove_callbacks()
- Don't log unraisable exceptions in case of interpreter shutdown
- Make sure we're not initialized if reinit fails
- Try to close the database even if ROLLBACK fails

(cherry picked from commit fd19509)

Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@erlend-aasland
Copy link
Contributor Author

Keeping this open until tests are added.

@serhiy-storchaka
Copy link
Member

sqlite3_close() can be made failing in many ways, but sqlite3_close_v2() tries hard to return SQLITE_OK, so it may be not so easy to provoke a failure.

https://www.sqlite.org/c3ref/close.html

@erlend-aasland
Copy link
Contributor Author

Yes, sqlite3_close_v2() returns SQLITE_OK if it is given a valid database handle.
sqlite3_close can return an error, but it is hard to see how to be able to trigger that from the constructor; I'm not sure it is possible. We could just as well use sqlite3_close_v2() in the constructor.

@vstinner
Copy link
Member

If closing a connection can fail but the sqlite3_close_v2() ignores errors, is there another API to "flush" a connection and reports errors?

If there is no easy way to trigger an error at exit or to report it, I suggest to just give up and close the issue.

@erlend-aasland
Copy link
Contributor Author

If there is no easy way to trigger an error at exit or to report it, I suggest to just give up and close the issue.

I don't think there is; the case is the constructor, between the sqlite3_open_v2 call and the following four goto error jumps. I don't see how we can provoke an error there.

Closing this as resolved.

@vstinner
Copy link
Member

Thanks, nice sqlite3 enhancement! Ignoring exceptions silently is never a good idea.

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

@erlend-aasland erlend-aasland

Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes topic-sqlite3 type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@vstinner @serhiy-storchaka @erlend-aasland