Skip to content

gh-80254: Disallow recursive usage of cursors in sqlite3 converters #29054

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.

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

Merged
merged 15 commits into from
May 3, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented 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
Copy link

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 19, 2021
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 19, 2021

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

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

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

@erlend-aasland erlend-aasland marked this pull request as ready for review October 24, 2021 21:34
@sir-sigurd
Copy link
Contributor

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

@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
Copy link
Contributor

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

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented May 2, 2022

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.

@miss-islington
Copy link
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.
🐍🍒⛏🤖

@miss-islington
Copy link
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

@miss-islington
Copy link
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

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

I'll fix the backports

@JelleZijlstra
Copy link
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?

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request May 3, 2022
…3` converters (python#29054)

(cherry picked from commit f629dcf)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@ambv
Copy link
Contributor

ambv commented May 4, 2022

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
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

@miss-islington
Copy link
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

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented May 4, 2022

@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).

@JelleZijlstra
Copy link
Member

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

@erlend-aasland
Copy link
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 pushed a commit that referenced this pull request May 5, 2022
…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 <fedoseev.sergey@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

* Fix ref leak in pysqlite_cursor_iternext
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sqlite crashes with converters mutating cursor
9 participants