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 — gh-80254: Disallow recursive usage of cursors in `sqlite3` converters by erlend-aasland · Pull Request #29054 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Oct 19, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Original PR: GH-11984

Fixes #80254

https://bugs.python.org/issue36073

Sorry, something went wrong.

Uh oh!

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

All reactions
sir-sigurd and others added 3 commits April 12, 2021 13:14
@sir-sigurd
bpo-36073: Raise ProgrammingError on recursive usage of cursors in sq…
c79e784
…lite converters
Erlend E. Aasland
Merge remote-tracking branch 'upstream/main' into sqlite-converter-se…
6c3bc37
…gfault
Erlend E. Aasland
Use subTest
bb0a729
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner October 19, 2021 09:48
@bedevere-bot bedevere-bot added the awaiting review label Oct 19, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 19, 2021
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 19, 2021
@bedevere-bot

bedevere-bot commented Oct 19, 2021

Copy link
Copy Markdown

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

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 19, 2021
@erlend-aasland
erlend-aasland requested a review from pablogsal October 19, 2021 09:49
@erlend-aasland

erlend-aasland commented Oct 19, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Buildbot comments:

  • buildbot/x86 Gentoo Installed with X PR: test_tk and test_ttk_guionly failed => unrelated
  • buildbot/x86 Gentoo Non-Debug with X PR: ditto
All reactions

Sorry, something went wrong.

Uh oh!

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

sir-sigurd
sir-sigurd reviewed Oct 19, 2021
View reviewed changes
Comment thread Lib/sqlite3/test/test_regression.py Resolved

Uh oh!

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

Comment thread Lib/sqlite3/test/test_regression.py Resolved

Uh oh!

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

@erlend-aasland
erlend-aasland marked this pull request as draft October 19, 2021 11:56
@erlend-aasland

erlend-aasland commented Oct 19, 2021

Copy link
Copy Markdown
Contributor Author

On hold until end of week: #29054 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

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

Erlend E. Aasland added 2 commits October 19, 2021 14:02
Erlend E. Aasland
Address review: Apply Sergey's suggestions
51ab551
Erlend E. Aasland
Also check exception message
f737b6e
@erlend-aasland
erlend-aasland marked this pull request as ready for review October 24, 2021 21:34
Erlend E. Aasland added 2 commits November 1, 2021 11:00
Erlend E. Aasland
Merge branch 'main' into sqlite-converter-segfault
94b48cb
Erlend E. Aasland
Merge branch 'main' into sqlite-converter-segfault
d3f6c94
@sir-sigurd sir-sigurd mentioned this pull request Nov 17, 2021
bpo-36073: Raise ProgrammingError on recursive usage of cursors in sqlite converters #11984
Closed
@sir-sigurd

sir-sigurd commented Nov 17, 2021

Copy link
Copy Markdown
Contributor

@erlend-aasland, sorry for delay, I closed my PR.

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

Erlend E. Aasland added 2 commits January 22, 2022 20:41
Erlend E. Aasland
Merge branch 'main' into sqlite-converter-segfault
2b8fddd
Erlend E. Aasland
Merge branch 'main' into sqlite-converter-segfault
119f800
kumaraditya303
kumaraditya303 reviewed Mar 5, 2022
View reviewed changes
Comment thread Lib/test/test_sqlite3/test_regression.py Resolved

Uh oh!

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

kumaraditya303
kumaraditya303 reviewed Mar 5, 2022
View reviewed changes
Comment thread Lib/test/test_sqlite3/test_regression.py Resolved

Uh oh!

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

@erlend-aasland erlend-aasland changed the title bpo-36073: Disallow recursive usage of cursors in sqlite3 converters gh-80254: Disallow recursive usage of cursors in sqlite3 converters Apr 10, 2022
@MaxwellDupre

MaxwellDupre commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

I think it would be a good idea to mention in the Docs. If already there please show ref.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
Sync with main
3f80f28
@erlend-aasland

erlend-aasland commented May 2, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I think it would be a good idea to mention in the Docs. If already there please show ref.

@MaxwellDupre, can you please explain why? I think we should not try to encourage users to shoot themselves in the foot with adapters and converters (or at all), so I prefer, very strongly, not to mention this in the docs.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
Make NEWS entry more accurate
ee8c8c2
@miss-islington

miss-islington commented May 3, 2022

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting merge label May 3, 2022
@miss-islington

miss-islington commented May 3, 2022

Copy link
Copy Markdown
Contributor

Sorry, @erlend-aasland and @JelleZijlstra, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f629dcfe835e349433e4c5099381d668e8fe69c8 3.10

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington miss-islington assigned JelleZijlstra May 3, 2022
@miss-islington

miss-islington commented May 3, 2022

Copy link
Copy Markdown
Contributor

Sorry @erlend-aasland and @JelleZijlstra, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker f629dcfe835e349433e4c5099381d668e8fe69c8 3.9

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
erlend-aasland deleted the sqlite-converter-segfault branch May 3, 2022 23:05
@erlend-aasland

erlend-aasland commented May 3, 2022

Copy link
Copy Markdown
Contributor Author

I'll fix the backports

All reactions

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra

JelleZijlstra commented May 3, 2022

Copy link
Copy Markdown
Member

Thanks! I think it should go into the security branches too, because every segfault is potentially an exploitable security issue. @ambv what do you think?

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request May 3, 2022
@erlend-aasland @sir-sigurd @JelleZijlstra
[3.10] pythongh-80254: Disallow recursive usage of cursors in `sqlite…
c908dc5
…3` converters (python#29054)

(cherry picked from commit f629dcf)

Co-authored-by: Sergey Fedoseev <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
@ambv

ambv commented May 4, 2022

Copy link
Copy Markdown
Contributor

@JelleZijlstra, correct! Crashers are DoS and as such are treated as security issues.

JelleZijlstra reacted with thumbs up emoji erlend-aasland reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

@ambv ambv added needs backport to 3.7 labels May 4, 2022
@miss-islington

miss-islington commented May 4, 2022

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented May 4, 2022

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented May 4, 2022

Copy link
Copy Markdown
Contributor

Sorry, @erlend-aasland and @JelleZijlstra, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f629dcfe835e349433e4c5099381d668e8fe69c8 3.8

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented May 4, 2022

Copy link
Copy Markdown
Contributor

Sorry @erlend-aasland and @JelleZijlstra, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker f629dcfe835e349433e4c5099381d668e8fe69c8 3.7

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 4, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@JelleZijlstra, correct! Crashers are DoS and as such are treated as security issues.

Thanks for that fact regarding crashers, I was unaware! 📝

I'll fix the backports as soon as possible (currently on my way back home from pycon).

All reactions

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra

JelleZijlstra commented May 4, 2022

Copy link
Copy Markdown
Member

Thanks! I looked at the 3.10 backport for a while but I'm not sure where the refleak is.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

Thanks! I looked at the 3.10 backport for a while but I'm not sure where the refleak is.

No worries, I found and fixed it in flight to Chicago. I've yet to push it though. But 3.9 contains even more ref. leaks! Looking into that soon.

JelleZijlstra reacted with laugh 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 removed needs backport to 3.7 release-blocker labels May 5, 2022
@erlend-aasland

erlend-aasland commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

FTR (also cross-posting this list on the issue):

  • 3.10 backport: [3.10] gh-80254: Disallow recursive usage of cursors in sqlite3 converters #92274
  • 3.9 backport: [3.9] gh-80254: Disallow recursive usage of cursors in sqlite3 converters #92278
  • 3.8 backport: [3.8] gh-80254: Disallow recursive usage of cursors in sqlite3 converters #92333
  • 3.7 backport: [3.7] gh-80254: Disallow recursive usage of cursors in sqlite3 converters #92334
All reactions

Sorry, something went wrong.

Uh oh!

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

JelleZijlstra pushed a commit that referenced this pull request May 5, 2022
@erlend-aasland
[3.10] gh-80254: Disallow recursive usage of cursors in sqlite3 con…
2a2421e
…verters (#92274)

* [3.10] gh-80254: Disallow recursive usage of cursors in `sqlite3` converters (#29054)

(cherry picked from commit f629dcf)

Co-authored-by: Sergey Fedoseev <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>

* Fix ref leak in pysqlite_cursor_iternext
@JelleZijlstra JelleZijlstra mentioned this pull request May 5, 2022
[3.8] gh-80254: Disallow recursive usage of cursors in sqlite3 converters #92333
Merged
@ned-deily ned-deily mentioned this pull request May 6, 2022
[3.7] gh-80254: Disallow recursive usage of cursors in sqlite3 converters #92334
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@pablogsal pablogsal Awaiting requested review from pablogsal
+1 more reviewer
@sir-sigurd sir-sigurd sir-sigurd left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@JelleZijlstra JelleZijlstra

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sqlite crashes with converters mutating cursor

Uh oh!

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

9 participants

@erlend-aasland @bedevere-bot @sir-sigurd @MaxwellDupre @JelleZijlstra @miss-islington @ambv @kumaraditya303 @the-knights-who-say-ni
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.