GeezerVison

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

GeezerVison is showing github.com — bpo-31764: Prevent a crash in sqlite3.Cursor.close() in case the Cursor object is uninitialized by orenmn · Pull Request #3958 · python/cpython

Conversation

orenmn commented Oct 11, 2017 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

In addition, add a test to test_sqlite to make sure that the crash is no more.

https://bugs.python.org/issue31764

Sorry, something went wrong.

Uh oh!

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

All reactions

orenmn added 2 commits

October 11, 2017 22:21


          init commit

c3bcda3


          improve the NEWS.d item

c07d198

the-knights-who-say-ni added the CLA signed label

Oct 11, 2017

bedevere-bot added the awaiting review label

Oct 11, 2017

orenmn commented Oct 11, 2017

Copy link

Copy Markdown

Contributor Author

I wonder whether we should add self->initialized = 0; after the call to PyArg_ParseTuple() in pysqlite_cursor_init(). (At least test_sqlite passes when i add this change.)
This would make sure that a Cursor object wouldn't be partially initialized while self->initialized == 1 .

However, i don't know how to produce code that would create a partially initialized Cursor object such that using it would cause problems.

All reactions

Sorry, something went wrong.

Uh oh!

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

berkerpeksag previously requested changes

Oct 12, 2017

View reviewed changes

Comment thread

Modules/_sqlite/cursor.c Outdated


		PyObject* pysqlite_cursor_close(pysqlite_Cursor* self, PyObject* args)
		{
		if (self->connection == NULL) {

berkerpeksag Oct 12, 2017

Copy link

Copy Markdown

Member

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.

Style nit: I think !foo is more common than foo == NULL in sqlite3 codebase. It would be nice to keep styling consistent within the module.

Sorry, something went wrong.

Uh oh!

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

orenmn reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Misc/NEWS.d/next/Library/2017-10-11-22-18-04.bpo-31764.EMyIkK.rst Outdated

		@@ -0,0 +1,2 @@
		Prevent a crash in `sqlite3.Cursor.close()` in case the `Cursor` object is

berkerpeksag Oct 12, 2017

Copy link

Copy Markdown

Member

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 use double backticks (``). I tried to explain why single backticks shouldn't be used at #3925 (comment)

Sorry, something went wrong.

Uh oh!

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

All reactions

orenmn Oct 12, 2017

Copy link

Copy Markdown

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

I am not familiar with Sphinx, but i trust you are :)
So maybe https://devguide.python.org/committing/#what-s-new-and-news-entries should also recommend double backticks?

Sorry, something went wrong.

Uh oh!

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

All reactions

berkerpeksag Oct 12, 2017

Copy link

Copy Markdown

Member

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.

Yeah, I wasn't aware of that example in the devguide. I will update it, thanks!

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/sqlite3/test/regression.py Outdated

		cur = Cursor(con)
		with self.assertRaises(sqlite.ProgrammingError):
		cur.execute("select 4+5").fetchall()
		self.assertRaises(sqlite.ProgrammingError, cur.close)

berkerpeksag Oct 12, 2017

Copy link

Copy Markdown

Member

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.

ProgrammingError is raised by a lot of things so I wonder if we should add a test for the exception message to make it more future-proof.

Sorry, something went wrong.

Uh oh!

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

All reactions

orenmn Oct 12, 2017

Copy link

Copy Markdown

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

All right. Should i also change the assertRaises() that was already in CheckCursorConstructorCallCheck()?

Sorry, something went wrong.

Uh oh!

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

All reactions

berkerpeksag Oct 12, 2017

Copy link

Copy Markdown

Member

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.

I'd say let's do it only for new additions to the test.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot commented Oct 12, 2017

Copy link

Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting changes and removed awaiting review labels

Oct 12, 2017

orenmn commented Oct 12, 2017

Copy link

Copy Markdown

Contributor Author

@berkerpeksag What do you think about #3958 (comment)?

All reactions

Sorry, something went wrong.

Uh oh!

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


          improve the test and the NEWS.d item, and make a style change in the …

25ff4f3

…C code.

serhiy-storchaka self-requested a review

October 12, 2017 16:54

berkerpeksag commented Oct 12, 2017

Copy link

Copy Markdown

Member

However, i don't know how to produce code that would create a partially initialized Cursor object such that using it would cause problems.

I agree with you. There is no need to add additional code for hypothetical use cases. We can change it later if we find a way to create a partially initialized Cursor object.

All reactions

Sorry, something went wrong.

Uh oh!

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


          don't change the old test in CheckCursorConstructorCallCheck

7e0cc4c

vstinner dismissed berkerpeksag’s stale review

November 7, 2017 00:09

The author took comments in account.

vstinner merged commit edb13ae into python:master

Nov 7, 2017

bedevere-bot removed the awaiting changes label

Nov 7, 2017

vstinner added the needs backport to 3.6 label

Nov 7, 2017

miss-islington commented Nov 7, 2017

Copy link

Copy Markdown

Contributor

Thanks @orenmn for the PR, and @Haypo for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Nov 7, 2017


          bpo-31764: Prevent a crash in sqlite3.Cursor.close() in case the Curs…

…or object is uninitialized (pythonGH-3958)

(cherry picked from commit edb13ae)

bedevere-bot removed the needs backport to 3.6 label

Nov 7, 2017

bedevere-bot commented Nov 7, 2017

Copy link

Copy Markdown

GH-4303 is a backport of this pull request to the 3.6 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner pushed a commit that referenced this pull request

Nov 7, 2017


          bpo-31764: Prevent a crash in sqlite3.Cursor.close() in case the Curs…

b0331c9

…or object is uninitialized (GH-3958) (#4303)

(cherry picked from commit edb13ae)

orenmn mentioned this pull request

Nov 8, 2017

[2.7] bpo-31764: Prevent a crash in sqlite3.Cursor.close() in case the Cursor object is uninitialized (GH-3958) #4333

Merged

embray pushed a commit to embray/cpython that referenced this pull request

Nov 9, 2017


          bpo-31764: Prevent a crash in sqlite3.Cursor.close() in case the Curs…

59a073a

…or object is uninitialized (python#3958)

lielfr mentioned this pull request

Jun 3, 2019

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

Closed

Reviewers

berkerpeksag

berkerpeksag left review comments

serhiy-storchaka

Awaiting requested review from serhiy-storchaka

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

6 participants

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.

Conversation

orenmn commented Oct 11, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orenmn commented Oct 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 12, 2017

Uh oh!

orenmn commented Oct 12, 2017

Uh oh!

berkerpeksag commented Oct 12, 2017

Uh oh!

miss-islington commented Nov 7, 2017

Uh oh!

bedevere-bot commented Nov 7, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

orenmn commented Oct 11, 2017 •

edited by bedevere-bot

Loading

Uh oh!

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