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.

Sign up for GitHub

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

Jump to bottom

bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resize(). #8756

Merged
merged 2 commits into from
Aug 16, 2018
Merged

bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resize(). #8756

merged 2 commits into from
Aug 16, 2018

Conversation

sir-sigurd
Copy link
Contributor

@sir-sigurd sir-sigurd commented Aug 13, 2018 •
edited by bedevere-bot
Loading

serhiy-storchaka
serhiy-storchaka reviewed Aug 14, 2018
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM, I left just few minor comments.

Modules/_csv.c Outdated
PyMem_Free(old_rec);
}
if (self->rec == NULL) {
size_t rec_size_new = (size_t)rec_len + MEM_INCR;
Copy link
Member

Choose a reason for hiding this comment

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

Why not (rec_len / MEM_INCR + 1) * MEM_INCR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot about integer division :-). I'll revert it.

Modules/_pickle.c Outdated
PyMem_RESIZE(self->memo, PyObject *, new_size);
if (self->memo == NULL) {
PyObject **memo_new = self->memo;
PyMem_Resize(memo_new, PyObject *, new_size);
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK currently there is no difference between PyMem_RESIZE and PyMem_Resize. But since this PR will be backported to old versions, I suggest to use PyMem_RESIZE.

Modules/_pickle.c Outdated
else
PyMem_RESIZE(self->marks, Py_ssize_t, alloc);
Py_ssize_t *marks_old = self->marks;
PyMem_Resize(self->marks, Py_ssize_t, alloc);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Aug 14, 2018
@serhiy-storchaka serhiy-storchaka merged commit 67b9cc8 into python:master Aug 16, 2018
@miss-islington
Copy link
Contributor

Thanks @sir-sigurd for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-8779 is a backport of this pull request to the 3.7 branch.

@bedevere-bot
Copy link

GH-8780 is a backport of this pull request to the 3.6 branch.

@miss-islington
Copy link
Contributor

Sorry, @sir-sigurd and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 67b9cc8e6072a919d2ed7e7ecc8124c8acfb3733 2.7

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 16, 2018
@sir-sigurd @miss-islington
bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resiz…
5955be5 
…e(). (pythonGH-8756)

(cherry picked from commit 67b9cc8)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 16, 2018
@sir-sigurd @miss-islington
bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resiz…
9e65d2f 
…e(). (pythonGH-8756)

(cherry picked from commit 67b9cc8)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
miss-islington added a commit that referenced this pull request Aug 16, 2018
@miss-islington @sir-sigurd
bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resiz…
962051e 
…e(). (GH-8756)

(cherry picked from commit 67b9cc8)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
miss-islington added a commit that referenced this pull request Aug 16, 2018
@miss-islington @sir-sigurd
bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resiz…
b14ab44 
…e(). (GH-8756)

(cherry picked from commit 67b9cc8)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
@serhiy-storchaka
Copy link
Member

Thank you for your PR @sir-sigurd. Nice catch.

Do you mind to make a backport to 2.7? It needs careful rewriting since int is used instead Py_ssize_t for indices and lengths.

@sir-sigurd sir-sigurd deleted the fix-mem-resize-leaks branch August 16, 2018 11:21
@bedevere-bot
Copy link

GH-8785 is a backport of this pull request to the 2.7 branch.

sir-sigurd
sir-sigurd commented Aug 16, 2018
View reviewed changes
Modules/_pickle.c
if (self->marks == NULL) {
PyMem_FREE(marks_old);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@serhiy-storchaka
Might make sense to not change self->marks at all instead of PyMem_FREE() when PyMem_RESIZE() fails?
It was implemented this way in 2.7:

cpython/Modules/cPickle.c

Lines 4661 to 4669 in 00aebab

if (self->marks == NULL)
marks=(Py_ssize_t *)malloc(s * sizeof(Py_ssize_t));
else
marks=(Py_ssize_t *)realloc(self->marks,
s * sizeof(Py_ssize_t));
if (!marks) {
PyErr_NoMemory();
return -1;
}

Copy link
Member

Choose a reason for hiding this comment

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

Agree, it makes sense.

carljm added a commit to carljm/cpython that referenced this pull request Aug 19, 2018
@carljm
Merge branch 'master' into cached-property
977ece0 
* master: (107 commits)
  bpo-22057: Clarify eval() documentation (pythonGH-8812)
  bpo-34318: Convert deprecation warnings to errors in assertRaises() etc. (pythonGH-8623)
  bpo-22602: Raise an exception in the UTF-7 decoder for ill-formed sequences starting with "+". (pythonGH-8741)
  bpo-34415: Updated logging.Formatter docstring. (pythonGH-8811)
  bpo-34432: doc Mention complex and decimal.Decimal on str.format not about locales (pythonGH-8808)
  bpo-34381: refer to 'Running & Writing Tests' in README.rst (pythonGH-8797)
  Improve error message when mock.assert_has_calls fails (pythonGH-8205)
  Warn not to set SIGPIPE to SIG_DFL (python#6773)
  bpo-34419: selectmodule.c does not compile on HP-UX due to bpo-31938 (pythonGH-8796)
  bpo-34418: Fix HTTPErrorProcessor documentation (pythonGH-8793)
  bpo-34391: Fix ftplib test for TLS 1.3 (pythonGH-8787)
  bpo-34217: Use lowercase for windows headers (pythonGH-8472)
  bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resize(). (pythonGH-8756)
  bpo-34405: Updated to OpenSSL 1.1.0i for Windows builds. (pythonGH-8775)
  bpo-34384: Fix os.readlink() on Windows (pythonGH-8740)
  closes bpo-34400: Fix undefined behavior in parsetok(). (pythonGH-4439)
  bpo-34399: 2048 bits RSA keys and DH params (python#8762)
  Make regular expressions in test_tasks.py raw strings. (pythonGH-8759)
  smtplib documentation fixes (pythonGH-8708)
  Fix misindented yaml in logging how to example (pythonGH-8604)
  ...
@serhiy-storchaka serhiy-storchaka removed their assignment Sep 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
skip news type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sir-sigurd @miss-islington @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni