bpo-31746: Fixed Segfaults in the sqlite module when uninitialized. #3946

lielfr · 2017-10-10T21:41:59Z

https://bugs.python.org/issue31746

the-knights-who-say-ni · 2017-10-10T21:42:01Z

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!

orenmn · 2017-10-11T05:24:05Z

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.

Modules/_sqlite/connection.c

Lib/sqlite3/test/regression.py

orenmn · 2017-10-11T11:29:18Z

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.


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

Lib/sqlite3/test/regression.py

Modules/_sqlite/connection.c

csabella · 2019-05-30T19:17:37Z

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

lielfr · 2019-06-03T09:42:54Z

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

csabella · 2019-06-03T11:12:59Z

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

lielfr · 2019-06-03T11:16:47Z

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.

csabella · 2019-06-03T12:18:32Z

@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! :-)

lielfr · 2019-07-02T05:11:53Z

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

erlend-aasland

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

Modules/_sqlite/connection.c

Lib/sqlite3/test/regression.py

lielfr · 2021-04-28T17:58:08Z

Sorry for the delay. Will fix this now.

erlend-aasland · 2021-04-28T17:59:28Z

Sorry for the delay. Will fix this now.

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

… made closing an uninitialized connection throw a ProgrammingError instead of segfaulting

lielfr · 2021-04-28T20:11:30Z

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

Modules/_sqlite/connection.c

erlend-aasland · 2021-04-30T09:33:56Z

Modules/_sqlite/connection.c

+    if (self == NULL) {
+        return NULL;
+    }
+
+    static char *kwlist[] = {"factory", NULL};


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

erlend-aasland · 2021-04-30T09:36:37Z

Modules/_sqlite/connection.c

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


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.

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.

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.

erlend-aasland · 2021-04-30T09:38:45Z

Modules/_sqlite/connection.c

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


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;

}

erlend-aasland · 2021-07-30T07:38:44Z

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

the-knights-who-say-ni added the CLA not signed label Oct 10, 2017

bedevere-bot added the awaiting review label Oct 10, 2017

orenmn reviewed Oct 11, 2017

View reviewed changes

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Oct 11, 2017

orenmn reviewed Oct 11, 2017

View reviewed changes

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved

orenmn reviewed Oct 11, 2017

View reviewed changes

Lib/sqlite3/test/regression.py Outdated Show resolved Hide resolved

orenmn reviewed Oct 11, 2017

View reviewed changes

berkerpeksag reviewed Oct 11, 2017

View reviewed changes

Lib/sqlite3/test/regression.py Outdated Show resolved Hide resolved

Modules/_sqlite/connection.c Show resolved Hide resolved

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 13, 2017

berkerpeksag added the skip news label Sep 27, 2018

brettcannon added the type-bug An unexpected behavior, bug, or error label Mar 29, 2019

brettcannon closed this Mar 29, 2019

brettcannon reopened this Mar 29, 2019

csabella requested a review from berkerpeksag July 2, 2019 14:28

erlend-aasland requested changes Apr 8, 2021

View reviewed changes

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved

Lib/sqlite3/test/regression.py Show resolved Hide resolved

lielfr added 3 commits April 28, 2021 21:13

bpo-31746: Fixed Segfaults in the sqlite module when uninitialized.

bbe3a0e

Added regression tests, changed RuntimeError to ProgrammingError, and…

b57a6af

… made closing an uninitialized connection throw a ProgrammingError instead of segfaulting

Fixing typo, identation and tests according to reviews.

9955edf

lielfr added 3 commits April 28, 2021 21:18

The first example code doesn't crash anymore.

c0920c8

No more crashes from python#31746

77b2946

Fixed C whitespace error

0fcd075

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

Fixing the files according to the feedback in the PR

d8b4dd6

Adding whitespace at the end of regression.py

51e7a9f

Fixing unnecessary extra whitespace in regression.py

84326a2

erlend-aasland requested changes Apr 30, 2021

View reviewed changes

erlend-aasland closed this Jul 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31746: Fixed Segfaults in the sqlite module when uninitialized. #3946

bpo-31746: Fixed Segfaults in the sqlite module when uninitialized. #3946

lielfr commented Oct 10, 2017 •

edited by bedevere-bot

Loading

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

orenmn commented Oct 11, 2017

orenmn Oct 11, 2017

csabella commented May 30, 2019

lielfr commented Jun 3, 2019

csabella commented Jun 3, 2019

lielfr commented Jun 3, 2019

csabella commented Jun 3, 2019

lielfr commented Jul 2, 2019

erlend-aasland left a comment

lielfr commented Apr 28, 2021

erlend-aasland commented Apr 28, 2021

lielfr commented Apr 28, 2021

erlend-aasland Apr 30, 2021

erlend-aasland Apr 30, 2021

lielfr May 2, 2021

erlend-aasland May 2, 2021

erlend-aasland Apr 30, 2021

erlend-aasland commented Jul 30, 2021

bpo-31746: Fixed Segfaults in the sqlite module when uninitialized. #3946

bpo-31746: Fixed Segfaults in the sqlite module when uninitialized. #3946

Conversation

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

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

orenmn commented Oct 11, 2017

orenmn Oct 11, 2017

Choose a reason for hiding this comment

csabella commented May 30, 2019

lielfr commented Jun 3, 2019

csabella commented Jun 3, 2019

lielfr commented Jun 3, 2019

csabella commented Jun 3, 2019

lielfr commented Jul 2, 2019

erlend-aasland left a comment

Choose a reason for hiding this comment

lielfr commented Apr 28, 2021

erlend-aasland commented Apr 28, 2021

lielfr commented Apr 28, 2021

erlend-aasland Apr 30, 2021

Choose a reason for hiding this comment

erlend-aasland Apr 30, 2021

Choose a reason for hiding this comment

lielfr May 2, 2021

Choose a reason for hiding this comment

erlend-aasland May 2, 2021

Choose a reason for hiding this comment

erlend-aasland Apr 30, 2021

Choose a reason for hiding this comment

erlend-aasland commented Jul 30, 2021

lielfr commented Oct 10, 2017 •

edited by bedevere-bot

Loading