Skip to content
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

bpo-1635741: Fix potential refleaks in binascii module #18613

Merged
merged 2 commits into from
Mar 11, 2020

Conversation

shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Feb 23, 2020

@codecov
Copy link

codecov bot commented Feb 23, 2020

Codecov Report

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

Impacted file tree graph

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

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

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

Copy link
Member Author

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.

@@ -1639,16 +1649,46 @@ static PyModuleDef_Slot binascii_slots[] = {
{0, NULL}
};

static int
binascii_traverse(PyObject *m, visitproc visit, void *arg)
Copy link
Member

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.

Copy link
Member Author

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.

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

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.

@vstinner
Copy link
Member

vstinner commented Mar 2, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants