GeezerVison

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

GeezerVison is showing github.com — bpo-1635741: Fix potential refleaks in binascii module by shihai1991 · Pull Request #18613 · python/cpython

Conversation

shihai1991 commented Feb 23, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

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

All reactions


          Fix potential refleaks in binascii module

3a3274a

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

Feb 23, 2020

bedevere-bot added the awaiting review label

Feb 23, 2020

codecov Bot commented Feb 23, 2020 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Codecov Report

Merging #18613 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18613      +/-   ##
==========================================
+ Coverage   82.06%   82.13%   +0.06%     
==========================================
  Files        1955     1955              
  Lines      584080   584621     +541     
  Branches    44458    44484      +26     
==========================================
+ Hits       479327   480176     +849     
+ Misses      95124    94794     -330     
- Partials     9629     9651      +22

Impacted Files	Coverage Δ
Lib/collections/__init__.py	`88.72% <0.00%> (-1.71%)`	⬇️
Lib/idlelib/colorizer.py	`85.78% <0.00%> (-1.02%)`	⬇️
Lib/test/test_capi.py	`88.70% <0.00%> (-0.73%)`	⬇️
Lib/test/test_asyncio/test_subprocess.py	`92.06% <0.00%> (-0.43%)`	⬇️
Objects/listobject.c	`90.77% <0.00%> (-0.29%)`	⬇️
Objects/stringlib/codecs.h	`97.33% <0.00%> (-0.27%)`	⬇️
Modules/clinic/mathmodule.c.h	`85.58% <0.00%> (-0.13%)`	⬇️
Lib/test/test_buffer.py	`95.14% <0.00%> (-0.11%)`	⬇️
Modules/_pickle.c	`74.66% <0.00%> (-0.09%)`	⬇️
Objects/dictobject.c	`86.97% <0.00%> (-0.05%)`	⬇️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbe2e0b...a2f1f99. Read the comment docs.

All reactions

Sorry, something went wrong.

Uh oh!

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

shihai1991 requested review from encukou and vstinner

February 23, 2020 08:13

vstinner reviewed

Feb 29, 2020

View reviewed changes

Comment thread

Modules/binascii.c Outdated

		binascii_clear(PyObject *m)
		{
		binascii_state *state = get_binascii_state(m);
		if (state == NULL) {

vstinner Feb 29, 2020

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 don't think that state can be NULL. If it's NULL, it's a bug in Python. No? Same remark in binascii_traverse().

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 Mar 1, 2020

Copy link

Copy Markdown

Member 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 do some debug in my vm:

Breakpoint 1, binascii_traverse (module=0x7ffff7e803d0, visit=0x4630b6 <bad_traverse_test>, arg=0x0) at /temp/shihai/cpython/Modules/binascii.c:1654
1654    {
(gdb) n
1655        binascii_state *state = get_binascii_state(module);
(gdb) n
1656        if (state == NULL) {
(gdb) n
1657            return -1;
(gdb) p state
$5 = (binascii_state *) 0x0
(gdb) c
Continuing.

Breakpoint 3, binascii_exec (module=0x7ffff7e803d0) at /temp/shihai/cpython/Modules/binascii.c:1615
1615    binascii_exec(PyObject *module) {

What's the reason?
the malloc operation of md_state will be called after first calling binascii_traverse.
https://github.com/python/cpython/blob/master/Objects/moduleobject.c#L399.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/binascii.c Outdated

		};

		static int
		binascii_traverse(PyObject m, visitproc visit, void arg)

vstinner Feb 29, 2020

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.

nitpick: I would prefer to use longer variable name than just "m". Rename "m" to "mod" or "module". Same remark if following functions.

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 Mar 1, 2020

Copy link

Copy Markdown

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

ok, no probleam, i will update it.

Sorry, something went wrong.

Uh oh!

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

All reactions

shihai1991 commented

Mar 1, 2020

View reviewed changes

Comment thread

Modules/binascii.c Outdated

		binascii_traverse(PyObject m, visitproc visit, void arg)
		{
		binascii_state *state = get_binascii_state(m);
		if (state == NULL) {

shihai1991 Mar 1, 2020

Copy link

Copy Markdown

Member 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 will update this behavior soon.

Sorry, something went wrong.

Uh oh!

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

All reactions


          using the var name module to replace m

a2f1f99

vstinner commented Mar 2, 2020

Copy link

Copy Markdown

Member

I created https://bugs.python.org/issue39824 to change the md_state==NULL case.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner added the skip news label

Mar 11, 2020

vstinner merged commit aa0c080 into python:master

Mar 11, 2020

bedevere-bot removed the awaiting review label

Mar 11, 2020

corona10 mentioned this pull request

May 12, 2021

Use get_binascii_state instead of PyModule_GetState #26069

Merged

kylotan mannequin mentioned this pull request

Sep 19, 2022

Py_Finalize() doesn't clear all Python objects at exit #44470

Closed

Reviewers

vstinner

vstinner left review comments

encukou

Awaiting requested review from encukou

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

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

GeezerVison

Conversation

shihai1991 commented Feb 23, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vstinner Feb 29, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Mar 1, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Feb 29, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Mar 1, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Mar 1, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner commented Mar 2, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shihai1991 commented Feb 23, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

codecov Bot commented Feb 23, 2020 •

edited

Loading

Uh oh!

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