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

sir-sigurd · 2018-08-13T16:54:18Z

https://bugs.python.org/issue34395

…e().

serhiy-storchaka

LGTM, I left just few minor comments.

serhiy-storchaka · 2018-08-14T17:43:28Z

Modules/_csv.c

-                PyMem_Free(old_rec);
-        }
-        if (self->rec == NULL) {
+        size_t rec_size_new = (size_t)rec_len + MEM_INCR;


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

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

serhiy-storchaka · 2018-08-14T17:47:42Z

Modules/_pickle.c

-    PyMem_RESIZE(self->memo, PyObject *, new_size);
-    if (self->memo == NULL) {
+    PyObject **memo_new = self->memo;
+    PyMem_Resize(memo_new, PyObject *, new_size);


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.

serhiy-storchaka · 2018-08-14T17:48:51Z

Modules/_pickle.c

-        else
-            PyMem_RESIZE(self->marks, Py_ssize_t, alloc);
+        Py_ssize_t *marks_old = self->marks;
+        PyMem_Resize(self->marks, Py_ssize_t, alloc);


Same as above.

miss-islington · 2018-08-16T04:27:52Z

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 · 2018-08-16T04:28:15Z

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

bedevere-bot · 2018-08-16T04:28:27Z

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

miss-islington · 2018-08-16T04:28:30Z

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

…e(). (pythonGH-8756) (cherry picked from commit 67b9cc8) Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>

…e(). (GH-8756) (cherry picked from commit 67b9cc8) Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>

serhiy-storchaka · 2018-08-16T08:51:17Z

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.

bedevere-bot · 2018-08-16T12:37:15Z

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

sir-sigurd · 2018-08-16T12:51:51Z

Modules/_pickle.c

+        Py_ssize_t *marks_old = self->marks;
+        PyMem_RESIZE(self->marks, Py_ssize_t, alloc);
        if (self->marks == NULL) {
+            PyMem_FREE(marks_old);


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

}

Agree, it makes sense.

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

bpo-34395: Fix memory leaks caused by incautious usage of PyMem_Resiz…

c2bb8a6

…e().

the-knights-who-say-ni added the CLA signed label Aug 13, 2018

bedevere-bot added the awaiting review label Aug 13, 2018

serhiy-storchaka reviewed Aug 14, 2018

View reviewed changes

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Aug 14, 2018

Restored some old behavior.

60ea005

serhiy-storchaka approved these changes Aug 16, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Aug 16, 2018

serhiy-storchaka added the skip news label Aug 16, 2018

serhiy-storchaka merged commit 67b9cc8 into python:master Aug 16, 2018

bedevere-bot removed the awaiting merge label Aug 16, 2018

bedevere-bot removed the needs backport to 3.7 label Aug 16, 2018

bedevere-bot removed the needs backport to 3.6 label Aug 16, 2018

miss-islington assigned serhiy-storchaka Aug 16, 2018

miss-islington added a commit that referenced this pull request Aug 16, 2018

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

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>

sir-sigurd deleted the fix-mem-resize-leaks branch August 16, 2018 11:21

bedevere-bot removed the needs backport to 2.7 label Aug 16, 2018

sir-sigurd commented Aug 16, 2018

View reviewed changes

serhiy-storchaka removed their assignment Sep 22, 2018

	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;
	}

Uh oh!

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

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

Uh oh!

Conversation

sir-sigurd commented Aug 13, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

sir-sigurd Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Aug 16, 2018

Uh oh!

bedevere-bot commented Aug 16, 2018

Uh oh!

bedevere-bot commented Aug 16, 2018

Uh oh!

miss-islington commented Aug 16, 2018

Uh oh!

serhiy-storchaka commented Aug 16, 2018

Uh oh!

bedevere-bot commented Aug 16, 2018

Uh oh!

sir-sigurd Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sir-sigurd commented Aug 13, 2018 •

edited by bedevere-bot

Loading