GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-31746: Fixed Segfaults in the sqlite module when uninitialized. by lielfr · Pull Request #3946 · python/cpython

Conversation

@lielfr

@lielfr lielfr commented Oct 10, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

https://bugs.python.org/issue31746

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni

the-knights-who-say-ni commented Oct 10, 2017

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Oct 10, 2017
@bedevere-bot bedevere-bot added the awaiting review label Oct 10, 2017
@orenmn

orenmn commented Oct 11, 2017

Copy link
Copy Markdown
Contributor

Oh, i was in the middle of writing my patch, but i guess you beat me to it :)

Anyway, IMHO we should add tests to verify that the crashes we know about don't happen anymore. (i am not sure, but Lib/sqlite3/test/regression.py looks like the right place.)

Also, i think that setting self->initialized to 1 in the beginning of pysqlite_connection_init() is something that could causes more problems when pysqlite_connection_init() fails in the middle, and leaves the Connection object partially initialized but marked as initialized.

BTW, i applied your patch, and calling close() on an uninitialized Connection object still crashes on my Windows 10.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
orenmn
orenmn reviewed Oct 11, 2017
View reviewed changes
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Oct 11, 2017
orenmn
orenmn reviewed Oct 11, 2017
View reviewed changes
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
orenmn
orenmn reviewed Oct 11, 2017
View reviewed changes
Comment thread Lib/sqlite3/test/regression.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
orenmn
orenmn reviewed Oct 11, 2017
View reviewed changes
Comment thread Lib/sqlite3/test/regression.py
def CheckUninitializedIsolationLevel(self):
"""
Previously trying to get the isolation level of an uninitialized Connection
caused a segfault, now it should just return None.

@orenmn orenmn Oct 11, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Other getters raise a ProgrammingError when they are called on an uninitialized Connection object.
IMHO this should be the same here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
berkerpeksag
berkerpeksag reviewed Oct 11, 2017
View reviewed changes
Comment thread Lib/sqlite3/test/regression.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 13, 2017
@berkerpeksag berkerpeksag added the skip news label Sep 27, 2018
@brettcannon brettcannon added the type-bug An unexpected behavior, bug, or error label Mar 29, 2019
@brettcannon brettcannon closed this Mar 29, 2019
@brettcannon brettcannon reopened this Mar 29, 2019
@csabella

csabella commented May 30, 2019

Copy link
Copy Markdown
Contributor

@lielfr, please fix the merge conflicts and, if you've addressed all the review comments, please mark them with the 'Resolve conversation' button. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lielfr

lielfr commented Jun 3, 2019

Copy link
Copy Markdown
Author

Hi @csabella, thanks for letting me know. I just thought the issue was fixed by @orenmn.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella

csabella commented Jun 3, 2019

Copy link
Copy Markdown
Contributor

@lielfr, I don't see where @orenmn worked on this issue. Since this pull request had had some activity, I was trying to move it forward by making sure the reviews had been addressed. Can you point out where @orenmn's changes are? Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lielfr

lielfr commented Jun 3, 2019

Copy link
Copy Markdown
Author

Well, PRs #3958 and #3968 seem to fix some of the issues. The testing code still crashes for some reason, so I'm going to fix that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella

csabella commented Jun 3, 2019

Copy link
Copy Markdown
Contributor

@lielfr - thank you for pointing those out. They were different issues on the bug tracker, so I didn't think the fix to one fixed the other (one was for cursors and one was for connections). Anyway, that would be great if you could take a look to see if this connection issue still happens and to fix those test issues you mentioned. Thank you for working on this! :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lielfr

lielfr commented Jul 2, 2019

Copy link
Copy Markdown
Author

Hi @csabella, It seems that this pr doesn't receive any attention from the maintainers. Should it just be left like that, or would you recommend reaching out to someone? Thanks again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella
csabella requested a review from berkerpeksag July 2, 2019 14:28
erlend-aasland
erlend-aasland requested changes Apr 8, 2021
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please fix the PEP 7 issues. Also, the tests are now missing. Could you rebase onto master and add them again?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/sqlite3/test/regression.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lielfr

lielfr commented Apr 28, 2021

Copy link
Copy Markdown
Author

Sorry for the delay. Will fix this now.
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Apr 28, 2021

Copy link
Copy Markdown
Contributor

Sorry for the delay. Will fix this now.

Great, there's no hurry :) Thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lielfr added 3 commits April 28, 2021 21:13
@lielfr
bpo-31746: Fixed Segfaults in the sqlite module when uninitialized.
bbe3a0e
@lielfr
Added regression tests, changed RuntimeError to ProgrammingError, and…
b57a6af 
… made closing an uninitialized connection throw a ProgrammingError instead of segfaulting
@lielfr
Fixing typo, identation and tests according to reviews.
9955edf
lielfr added 3 commits April 28, 2021 21:18
@lielfr
The first example code doesn't crash anymore.
c0920c8
@lielfr
No more crashes from python#31746
77b2946
@lielfr
Fixed C whitespace error
0fcd075
@lielfr
lielfr force-pushed the fix-issue-31746 branch from fd415af to 0fcd075 Compare April 28, 2021 18:19
lielfr added 3 commits April 28, 2021 23:07
@lielfr
Fixing the files according to the feedback in the PR
d8b4dd6
@lielfr
Adding whitespace at the end of regression.py
51e7a9f
@lielfr
Fixing unnecessary extra whitespace in regression.py
84326a2
@lielfr

lielfr commented Apr 28, 2021

Copy link
Copy Markdown
Author

Should be done. I'll be happy to fix other issues if there are any.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland requested changes Apr 30, 2021
View reviewed changes
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/connection.c
Comment on lines +299 to +303
if (self == NULL) {
return NULL;
}

static char *kwlist[] = {"factory", NULL};

@erlend-aasland erlend-aasland Apr 30, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is not needed if self->initialized is only set if pysqlite_connection_init was successful. Also, it looks like the rebase left some artifacts :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_sqlite/connection.c
Comment on lines +346 to +351

if (!self->statements) {
PyErr_SetString(pysqlite_ProgrammingError,
"Base Connection.__init__ not called.");
return NULL;
}

@erlend-aasland erlend-aasland Apr 30, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Wouldn't it be better to handle this in pysqlite_do_all_statements, as it has multiple callers? Perhaps pysqlite_do_all_statements could be split in two: do_all_statements and do_all_cursors. Each of them would do a pointer sanity check before looping.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lielfr lielfr May 2, 2021

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Are you sure about splitting pysqlite_do_all_statements into two functions? It seems like it always performs the statements part, but optionally also performs the cursors part.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland May 2, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No, I'm not sure :) Let's leave it as it is; both pysqlite_do_all_statements and this particular change.

Nit: if (self->statements == NULL) is more explicit, and I believe more consistent with the rest of the code base.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_sqlite/connection.c
Comment on lines +1254 to +1258
if (!self || !self->isolation_level) {
PyErr_Format(pysqlite_ProgrammingError,
"Object is null or isolation_level is uninitialized.");
return 0;
}

@erlend-aasland erlend-aasland Apr 30, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If self->initialized is only set if pysqlite_connection_init was successful, this can be reduced to:

Suggested change
if (!self || !self->isolation_level) {
PyErr_Format(pysqlite_ProgrammingError,
"Object is null or isolation_level is uninitialized.");
return 0;
}
if (!pysqlite_check_connection(self)) {
return NULL;
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@erlend-aasland

erlend-aasland commented Jul 30, 2021

Copy link
Copy Markdown
Contributor

Closing, since #27431 is now merged. Thank you for your PR, and interest in improving CPython!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland closed this Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland requested changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
+1 more reviewer
@orenmn orenmn orenmn left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@lielfr @the-knights-who-say-ni @orenmn @csabella @erlend-aasland @berkerpeksag @brettcannon @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.