gh-104265 Disallow instantiation of `_csv.Reader` and `_csv.Writer` #104266

chgnrdv · 2023-05-07T12:33:16Z

Set Py_TPFLAGS_DISALLOW_INSTANTIATION and unset Py_TPFLAGS_BASETYPE flags on Reader and Writer types to prevent their instantiation and subtyping

Issue: _csv Reader and Writer types shouldn't be directly instantiable #104265

…r` types Set `Py_TPFLAGS_DISALLOW_INSTANTIATION` and unset `Py_TPFLAGS_BASETYPE` flags on `Reader` and `Writer` types to prevent their instantiation and subtyping

erlend-aasland · 2023-05-07T13:43:27Z

Thanks! This needs a NEWS entry, preferrably with a reference to when the bug was introduced. I think this should be backported through to 3.10.

erlend-aasland · 2023-05-07T13:44:16Z

Also, please add tests. There's a dedicated test.support helper for this. Grep through the code base to find it and see how it's used.

chgnrdv · 2023-05-07T14:04:24Z

@erlend-aasland, are you talking about assert_python_ok? Can't we just check if TypeError with proper message is raised on attempt to create instance/subtype?

erlend-aasland · 2023-05-07T14:19:07Z

There's a disallow_instantiation helper in test.support.

erlend-aasland · 2023-05-07T14:46:14Z

Thanks! This needs a NEWS entry, preferrably with a reference to when the bug was introduced. I think this should be backported through to 3.10.

Ah, 3.10 is in security fix mode, so we can only backport to 3.11.

erlend-aasland · 2023-05-07T15:38:47Z

Some history, the original issue for applying PEP-384 to the _csv extension module:

A later (duplicate issue); the linked PRs were closed and incorporated into gh-23224

As you can see, both the Writer and the Reader class had Py_TPFLAGS_BASETYPE when they were converted from static to heap types. Removing that flag is a backwards incompatible change, as third-party software may depend on that behaviour. Please revert that particular change.

erlend-aasland

Changes requested:

Revert Py_TPFLAGS_BASETYPE changes
Add NEWS entry

bedevere-bot · 2023-05-07T15:40:55Z

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.

chgnrdv · 2023-05-07T15:52:42Z

Looks like 6a02b38 from #23224 introduced this issue. How can I reflect this in NEWS entry text? Should I specify an issue number?

erlend-aasland · 2023-05-07T16:15:10Z

Looks like 6a02b38 from #23224 introduced this issue. How can I reflect this in NEWS entry text? Should I specify an issue number?

Indeed it did.

You can reference GitHub issues/PRs with the :gh: Sphinx directive. Here's a suggestion to a NEWS entry:

Prevent possible crash by disallow instantiation of the :class:`!_csv.Reader`
and :class:`!_csv.Writer` types. The regression was introduced by :gh:`23224`
in Python 3.10. Patch by <your-name-here>.

chgnrdv · 2023-05-07T19:58:20Z

I have made the requested changes; please review again

bedevere-bot · 2023-05-07T19:58:24Z

Thanks for making the requested changes!

@erlend-aasland: please review the changes made to this pull request.

Misc/NEWS.d/next/Library/2023-05-07-19-56-45.gh-issue-104265.fVblry.rst

…Vblry.rst

Lib/test/test_csv.py

…ub.com:chgnrdv/cpython into _csv-make-Reader-Writer-types-non-instantiable

erlend-aasland · 2023-05-07T21:14:56Z

Thank you so much for the report and bugfix, Radislav; good job!

miss-islington · 2023-05-07T21:15:48Z

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

bedevere-bot · 2023-05-07T21:16:15Z

GH-104278 is a backport of this pull request to the 3.11 branch.

…ter` (pythonGH-104266) (cherry picked from commit 06c2a48) Co-authored-by: chgnrdv <52372310+chgnrdv@users.noreply.github.com>

chgnrdv · 2023-05-07T21:18:33Z

@erlend-aasland, thank you for your patient review :)

…iter` (GH-104266) (#104278) gh-104265 Disallow instantiation of `_csv.Reader` and `_csv.Writer` (GH-104266) (cherry picked from commit 06c2a48) Co-authored-by: chgnrdv <52372310+chgnrdv@users.noreply.github.com>

…ter` (python#104266)

* main: (47 commits) pythongh-97696 Remove unnecessary check for eager_start kwarg (python#104188) pythonGH-104308: socket.getnameinfo should release the GIL (python#104307) pythongh-104310: Add importlib.util.allowing_all_extensions() (pythongh-104311) pythongh-99113: A Per-Interpreter GIL! (pythongh-104210) pythonGH-104284: Fix documentation gettext build (python#104296) pythongh-89550: Buffer GzipFile.write to reduce execution time by ~15% (python#101251) pythongh-104223: Fix issues with inheriting from buffer classes (python#104227) pythongh-99108: fix typo in Modules/Setup (python#104293) pythonGH-104145: Use fully-qualified cross reference types for the bisect module (python#104172) pythongh-103193: Improve `getattr_static` test coverage (python#104286) Trim trailing whitespace and test on CI (python#104275) pythongh-102500: Remove mention of bytes shorthand (python#104281) pythongh-97696: Improve and fix documentation for asyncio eager tasks (python#104256) pythongh-99108: Replace SHA3 implementation HACL* version (python#103597) pythongh-104273: Remove redundant len() calls in argparse function (python#104274) pythongh-64660: Don't hardcode Argument Clinic return converter result variable name (python#104200) pythongh-104265 Disallow instantiation of `_csv.Reader` and `_csv.Writer` (python#104266) pythonGH-102613: Improve performance of `pathlib.Path.rglob()` (pythonGH-104244) pythongh-103650: Fix perf maps address format (python#103651) pythonGH-89812: Churn `pathlib.Path` methods (pythonGH-104243) ...

Disallow instantiation and subtyping of _csv.Reader and `_csv.Write…

ae69f9b

…r` types Set `Py_TPFLAGS_DISALLOW_INSTANTIATION` and unset `Py_TPFLAGS_BASETYPE` flags on `Reader` and `Writer` types to prevent their instantiation and subtyping

bedevere-bot added the awaiting review label May 7, 2023

chgnrdv changed the title ~~Disallow instantiation and subtyping of _csv.Reader and _csv.Writer~~ gh-104265 Disallow instantiation and subtyping of _csv.Reader and _csv.Writer May 7, 2023

bedevere-bot mentioned this pull request May 7, 2023

_csv Reader and Writer types shouldn't be directly instantiable #104265

Closed

corona10 requested a review from erlend-aasland May 7, 2023 13:34

added test

7d1fcd8

changed tests to use check_disallow_instantiation, separated tests

aa34fae

erlend-aasland added the needs backport to 3.11 only security fixes label May 7, 2023

erlend-aasland changed the title ~~gh-104265 Disallow instantiation and subtyping of _csv.Reader and _csv.Writer~~ gh-104265 Disallow instantiation of _csv.Reader and _csv.Writer May 7, 2023

erlend-aasland requested changes May 7, 2023

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels May 7, 2023

reverted Py_TPFLAGS_BASETYPE flag deletion, removed corresponding tests

6c45ff8

📜🤖 Added by blurb_it.

7d9ec2a

chgnrdv added 3 commits May 7, 2023 19:24

Update 2023-05-07-16-13-07.gh-issue-104265.fU60bZ.rst

19bddd0

Update 2023-05-07-16-13-07.gh-issue-104265.fU60bZ.rst

b3f4a65

Update 2023-05-07-16-13-07.gh-issue-104265.fU60bZ.rst

a022e91

chgnrdv requested a review from erlend-aasland May 7, 2023 16:49

blurb-it bot and others added 2 commits May 7, 2023 19:56

📜🤖 Added by blurb_it.

52aacfd

Delete 2023-05-07-16-13-07.gh-issue-104265.fU60bZ.rst

d31d214

bedevere-bot added awaiting change review and removed awaiting changes labels May 7, 2023

erlend-aasland reviewed May 7, 2023

View reviewed changes

Misc/NEWS.d/next/Library/2023-05-07-19-56-45.gh-issue-104265.fVblry.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Library/2023-05-07-19-56-45.gh-issue-104265.f…

2804744

…Vblry.rst

erlend-aasland reviewed May 7, 2023

View reviewed changes

Lib/test/test_csv.py Outdated Show resolved Hide resolved

chgnrdv added 2 commits May 7, 2023 23:49

applied suggestion by erlend-aasland

8255467

Merge branch '_csv-make-Reader-Writer-types-non-instantiable' of gith…

73609a8

…ub.com:chgnrdv/cpython into _csv-make-Reader-Writer-types-non-instantiable

erlend-aasland approved these changes May 7, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 7, 2023

erlend-aasland enabled auto-merge (squash) May 7, 2023 21:14

erlend-aasland merged commit 06c2a48 into python:main May 7, 2023

bedevere-bot removed the awaiting merge label May 7, 2023

bedevere-bot removed the needs backport to 3.11 only security fixes label May 7, 2023

jbower-fb pushed a commit to jbower-fb/cpython-jbowerfb that referenced this pull request May 8, 2023

pythongh-104265 Disallow instantiation of _csv.Reader and `_csv.Wri…

8e76e2d

…ter` (python#104266)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-104265 Disallow instantiation of `_csv.Reader` and `_csv.Writer` #104266

gh-104265 Disallow instantiation of `_csv.Reader` and `_csv.Writer` #104266

chgnrdv commented May 7, 2023 •

edited by bedevere-bot

Loading

erlend-aasland commented May 7, 2023

erlend-aasland commented May 7, 2023

chgnrdv commented May 7, 2023 •

edited

Loading

erlend-aasland commented May 7, 2023

erlend-aasland commented May 7, 2023

erlend-aasland commented May 7, 2023

erlend-aasland left a comment •

edited

Loading

bedevere-bot commented May 7, 2023

chgnrdv commented May 7, 2023

erlend-aasland commented May 7, 2023

chgnrdv commented May 7, 2023

bedevere-bot commented May 7, 2023

erlend-aasland commented May 7, 2023

miss-islington commented May 7, 2023

bedevere-bot commented May 7, 2023

chgnrdv commented May 7, 2023

gh-104265 Disallow instantiation of _csv.Reader and _csv.Writer #104266

gh-104265 Disallow instantiation of _csv.Reader and _csv.Writer #104266

Conversation

chgnrdv commented May 7, 2023 • edited by bedevere-bot Loading

erlend-aasland commented May 7, 2023

erlend-aasland commented May 7, 2023

chgnrdv commented May 7, 2023 • edited Loading

erlend-aasland commented May 7, 2023

erlend-aasland commented May 7, 2023

erlend-aasland commented May 7, 2023

erlend-aasland left a comment • edited Loading

Choose a reason for hiding this comment

bedevere-bot commented May 7, 2023

chgnrdv commented May 7, 2023

erlend-aasland commented May 7, 2023

chgnrdv commented May 7, 2023

bedevere-bot commented May 7, 2023

erlend-aasland commented May 7, 2023

miss-islington commented May 7, 2023

bedevere-bot commented May 7, 2023

chgnrdv commented May 7, 2023

gh-104265 Disallow instantiation of `_csv.Reader` and `_csv.Writer` #104266

gh-104265 Disallow instantiation of `_csv.Reader` and `_csv.Writer` #104266

chgnrdv commented May 7, 2023 •

edited by bedevere-bot

Loading

chgnrdv commented May 7, 2023 •

edited

Loading

erlend-aasland left a comment •

edited

Loading