FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-83004: Harden msvcrt init by erlend-aasland · Pull Request #103383 · python/cpython · GitHub

/ cpython Public

gh-83004: Harden msvcrt init - #103383

Merged
kumaraditya303 merged 3 commits into
python:mainfrom
erlend-aasland:cleanup-msvcrt
Apr 10, 2023
ConversationCommits (3)ChecksFiles changed
Merged

gh-83004: Harden msvcrt init#103383
kumaraditya303 merged 3 commits into
python:mainfrom
erlend-aasland:cleanup-msvcrt

Conversation

erlend-aasland commented Apr 8, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

erlend-aasland requested a review from a team as a code owner April 8, 2023 20:00
bedevere-bot mentioned this pull request Apr 8, 2023
Closed
erlend-aasland mentioned this pull request Apr 8, 2023
Merged
erlend-aasland marked this pull request as draft April 8, 2023 22:35
erlend-aasland marked this pull request as ready for review April 8, 2023 23:03
kumaraditya303 merged commit c3cd3d1 into python:main Apr 10, 2023
kumaraditya303 deleted the cleanup-msvcrt branch April 10, 2023 17:09

erlend-aasland commented Apr 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Darn, I forgot to add error handling for PyDict_SetItemString. I'll create a follow-up PR for that: gh-103420

zooba reviewed Apr 11, 2023
View reviewed changes
Comment thread PC/msvcrtmodule.c
Comment on lines +673 to +676
if (version == NULL) {
goto error;
}
st = PyModule_AddObjectRef(m, "CRT_ASSEMBLY_VERSION", version);

zooba Apr 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@erlend-aasland Just FYI, the modsupport functions allow passing in NULL provided an error has already occurred, and they just let it chain. So the code here was fine before.

No need to revert now though.

erlend-aasland Apr 11, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

TIL! Sorry for not waiting on your review.

erlend-aasland Apr 11, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

However, there was a leak if PyModule_AddObject failed, since that API only eat refs if successful.

kumaraditya303 Apr 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry I should have been more careful in this.

warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
pythongh-83004: Harden msvcrt init (python#103383)
1ce1aad
aisk pushed a commit to aisk/cpython that referenced this pull request Apr 18, 2023
pythongh-83004: Harden msvcrt init (python#103383)
ce07012
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

zooba zooba left review comments
kumaraditya303 kumaraditya303 approved these changes
hauntsaninja Awaiting requested review from hauntsaninja

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL