Skip to content

bpo-1635741: Enable unicode_release_interned() without insure or valgrind. #21087

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

Closed
shihai1991 wants to merge 9 commits into from
Closed

bpo-1635741: Enable unicode_release_interned() without insure or valgrind. #21087

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ed3cd98
Enable unicode_release_interned() without insure or valgrind.
shihai1991 Jun 23, 2020
3c8db50
resolve use after free
shihai1991 Jun 23, 2020
03f650d
Merge branch 'master' into bpo_1635741_interned_unicode
shihai1991 Jun 23, 2020
b2e578d
resolve confilicts
shihai1991 Jun 24, 2020
4618783
Use PyDict_Next() in unicode_release_interned()
shihai1991 Jun 25, 2020
8b47de1
resolve conflicts
shihai1991 Jun 25, 2020
33272ac
Merge remote-tracking branch 'origin/master' into bpo_1635741_interne…
shihai1991 Jun 25, 2020
ec1671b
update vars' name
shihai1991 Jun 25, 2020
b0da3bd
_PyUnicode_Fini() before _PyDict_Fini()
shihai1991 Jul 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 20 additions & 39 deletions Objects/unicodeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15657,65 +15657,57 @@ PyUnicode_InternFromString(const char *cp)
}


#if defined(WITH_VALGRIND) || defined(__INSURE__)
static void
unicode_release_interned(void)
{
Py_ssize_t pos = 0;
PyObject *key, *value;

if (interned == NULL || !PyDict_Check(interned)) {
return;
}
PyObject *keys = PyDict_Keys(interned);
if (keys == NULL || !PyList_Check(keys)) {
PyErr_Clear();
return;
}

/* Since unicode_release_interned() is intended to help a leak
detector, interned unicode strings are not forcibly deallocated;
rather, we give them their stolen references back, and then clear
and DECREF the interned dict. */

Py_ssize_t n = PyList_GET_SIZE(keys);
#ifdef INTERNED_STATS
fprintf(stderr, "releasing %zd interned strings\n", n);
fprintf(stderr, "releasing %zd interned strings\n", PyDict_Size(interned));

Py_ssize_t immortal_size = 0, mortal_size = 0;
#endif
for (Py_ssize_t i = 0; i < n; i++) {
PyObject *s = PyList_GET_ITEM(keys, i);
if (PyUnicode_READY(s) == -1) {
while (PyDict_Next(interned, &pos, &key, &value)) {
Comment thread
shihai1991 marked this conversation as resolved.
Outdated
if (PyUnicode_READY(key) == -1) {
Py_UNREACHABLE();
}
switch (PyUnicode_CHECK_INTERNED(s)) {
case SSTATE_INTERNED_IMMORTAL:
Py_SET_REFCNT(s, Py_REFCNT(s) + 1);
switch (PyUnicode_CHECK_INTERNED(key)) {
case SSTATE_INTERNED_IMMORTAL:
Py_SET_REFCNT(key, Py_REFCNT(key) + 1);
#ifdef INTERNED_STATS
immortal_size += PyUnicode_GET_LENGTH(s);
immortal_size += PyUnicode_GET_LENGTH(key);
#endif
break;
case SSTATE_INTERNED_MORTAL:
Py_SET_REFCNT(s, Py_REFCNT(s) + 2);
break;
case SSTATE_INTERNED_MORTAL:
Py_SET_REFCNT(key, Py_REFCNT(key) + 2);
#ifdef INTERNED_STATS
mortal_size += PyUnicode_GET_LENGTH(s);
mortal_size += PyUnicode_GET_LENGTH(key);
#endif
break;
case SSTATE_NOT_INTERNED:
/* fall through */
default:
Py_UNREACHABLE();
break;
case SSTATE_NOT_INTERNED:
/* fall through */
default:
Py_UNREACHABLE();
}
_PyUnicode_STATE(s).interned = SSTATE_NOT_INTERNED;
_PyUnicode_STATE(key).interned = SSTATE_NOT_INTERNED;
}
#ifdef INTERNED_STATS
fprintf(stderr,
"total size of all interned strings: %zd/%zd mortal/immortal\n",
mortal_size, immortal_size);
#endif
Py_DECREF(keys);
PyDict_Clear(interned);
Py_CLEAR(interned);
}
#endif


/********************* Unicode Iterator **************************/
Expand Down Expand Up @@ -16209,18 +16201,7 @@ _PyUnicode_Fini(PyThreadState *tstate)

int is_main_interp = _Py_IsMainInterpreter(tstate);
if (is_main_interp) {
#if defined(WITH_VALGRIND) || defined(__INSURE__)
/* Insure++ is a memory analysis tool that aids in discovering
* memory leaks and other memory problems. On Python exit, the
* interned string dictionaries are flagged as being in use at exit
* (which it is). Under normal circumstances, this is fine because
* the memory will be automatically reclaimed by the system. Under
* memory debugging, it's a huge source of useless noise, so we
* trade off slower shutdown for less distraction in the memory
* reports. -baw
*/
unicode_release_interned();
#endif /* __INSURE__ */
}

Py_CLEAR(state->empty);
Expand Down
2 changes: 1 addition & 1 deletion Python/pylifecycle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1260,13 +1260,13 @@ finalize_interp_types(PyThreadState *tstate, int is_main_interp)
_PyContext_Fini(tstate);

_PyDict_Fini(tstate);
_PyList_Fini(tstate);
_PyTuple_Fini(tstate);

_PySlice_Fini(tstate);

_PyBytes_Fini(tstate);
_PyUnicode_Fini(tstate);
_PyList_Fini(tstate);
Copy link
Copy Markdown
Member

@vstinner vstinner Jun 23, 2020

Choose a reason for hiding this comment

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

Why do you clear list after unicode?

The rationale of the current code is that a list can contains slice, unicode, etc. So list must be cleared first.

Copy link
Copy Markdown
Member Author

@shihai1991 shihai1991 Jun 23, 2020

Choose a reason for hiding this comment

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

oh the assert failed: python: Objects/listobject.c:144: PyList_New: Assertion `state->numfree != -1' failed.

the traceback:

#0  0x00007ffff6ce2207 in raise () from /lib64/libc.so.6
#1  0x00007ffff6ce38f8 in abort () from /lib64/libc.so.6
#2  0x00007ffff6cdb026 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6cdb0d2 in __assert_fail () from /lib64/libc.so.6
#4  0x0000000000446b7f in PyList_New (size=size@entry=1524) at ./Include/internal/pycore_pystate.h:95
#5  0x00000000004560c3 in dict_keys (mp=mp@entry=0x7ffff7fa70b0) at Objects/dictobject.c:2196
#6  0x000000000045e16f in PyDict_Keys (mp=0x7ffff7fa70b0) at Objects/dictobject.c:2758
#7  0x00000000004b1f34 in unicode_release_interned () at Objects/unicodeobject.c:15666
#8  0x00000000004def2a in _PyUnicode_Fini (tstate=tstate@entry=0xa29a60) at Objects/unicodeobject.c:16207
#9  0x0000000000531adb in finalize_interp_types (tstate=tstate@entry=0xa29a60, is_main_interp=is_main_interp@entry=1) at Python/pylifecycle.c:1272
#10 0x0000000000531b62 in finalize_interp_clear (tstate=tstate@entry=0xa29a60) at Python/pylifecycle.c:1305
#11 0x0000000000534591 in Py_FinalizeEx () at Python/pylifecycle.c:1475
#12 0x000000000041d6ef in Py_RunMain () at Modules/main.c:634
#13 0x000000000041d73f in pymain_main (args=args@entry=0x7fffffffe2c0) at Modules/main.c:662
#14 0x000000000041d7bb in Py_BytesMain (argc=<optimized out>, argv=<optimized out>) at Modules/main.c:686
#15 0x000000000041c33d in main (argc=<optimized out>, argv=<optimized out>) at ./Programs/python.c:15

Copy link
Copy Markdown
Member Author

@shihai1991 shihai1991 Jun 23, 2020

Choose a reason for hiding this comment

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

copy from the failed ci gate:
python: Objects/dictobject.c:610: free_keys_object: Assertion `state->keys_numfree != -1' failed.

Copy link
Copy Markdown
Member

@vstinner vstinner Jun 24, 2020

Choose a reason for hiding this comment

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

Hum, maybe unicode_release_interned() must use PyDict_Next() to iterate on the dict, rather than calling PyDict_Keys() which creates a new list.

Copy link
Copy Markdown
Member Author

@shihai1991 shihai1991 Jun 24, 2020

Choose a reason for hiding this comment

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

Thanks, I will try it.

Copy link
Copy Markdown
Member Author

@shihai1991 shihai1991 Jun 25, 2020

Choose a reason for hiding this comment

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

this line can not reverted now, because many assert failed in dictobject.c:

#0  0x00007ffff6ce2207 in raise () from /lib64/libc.so.6
#1  0x00007ffff6ce38f8 in abort () from /lib64/libc.so.6
#2  0x00007ffff6cdb026 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6cdb0d2 in __assert_fail () from /lib64/libc.so.6
#4  0x000000000045647d in free_keys_object (keys=keys@entry=0x7ffff7e41030) at Objects/dictobject.c:616
#5  0x000000000045c8a4 in dictkeys_decref (dk=0x7ffff7e41030) at Objects/dictobject.c:336
#6  PyDict_Clear (op=0x7ffff7fa80b0) at Objects/dictobject.c:1767
#7  0x00000000004b23c3 in unicode_release_interned () at Objects/unicodeobject.c:15708
#8  0x00000000004deeb9 in _PyUnicode_Fini (tstate=tstate@entry=0xa2a7c0) at Objects/unicodeobject.c:16204
#9  0x0000000000531a7e in finalize_interp_types (tstate=tstate@entry=0xa2a7c0, is_main_interp=is_main_interp@entry=1) at Python/pylifecycle.c:1269
#10 0x0000000000531ae6 in finalize_interp_clear (tstate=tstate@entry=0xa2a7c0) at Python/pylifecycle.c:1298

#0  0x00007ffff6ce2207 in raise () from /lib64/libc.so.6
#1  0x00007ffff6ce38f8 in abort () from /lib64/libc.so.6
#2  0x00007ffff6cdb026 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6cdb0d2 in __assert_fail () from /lib64/libc.so.6
#4  0x0000000000457e15 in dict_dealloc (mp=0x7ffff7fa80b0) at Objects/dictobject.c:336
#5  0x000000000046a0c9 in _Py_Dealloc (op=<optimized out>) at Objects/object.c:2211
#6  0x00000000004b22d6 in _Py_DECREF (op=<optimized out>, lineno=15709, filename=0x6a3552 "Objects/unicodeobject.c") at ./Include/object.h:449
#7  unicode_release_interned () at Objects/unicodeobject.c:15709
#8  0x00000000004ded7b in _PyUnicode_Fini (tstate=tstate@entry=0xa2a7c0) at Objects/unicodeobject.c:16204
#9  0x0000000000531942 in finalize_interp_types (tstate=tstate@entry=0xa2a7c0, is_main_interp=is_main_interp@entry=1) at Python/pylifecycle.c:1269
#10 0x00000000005319aa in finalize_interp_clear (tstate=tstate@entry=0xa2a7c0) at Python/pylifecycle.c:1298

Copy link
Copy Markdown
Member Author

@shihai1991 shihai1991 Jun 25, 2020

Choose a reason for hiding this comment

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

could we remove those asserts? I don't know there are some risks or not after we remove those asserts.

Copy link
Copy Markdown
Member Author

@shihai1991 shihai1991 Jun 25, 2020

Choose a reason for hiding this comment

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

The above traceback is about PyDict_Fini(), not PyList_Fini().

Copy link
Copy Markdown
Member Author

@shihai1991 shihai1991 Jun 25, 2020

Choose a reason for hiding this comment

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

The rationale of the current code is that a list can contains slice, unicode, etc. So list must be cleared first.

Hi, victor. I have a question about it: if we broke the clear order, there will be some resource leak in here?

Copy link
Copy Markdown
Member Author

@shihai1991 shihai1991 Jul 1, 2020

Choose a reason for hiding this comment

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

Or _PyUnicode_Fini() before _PyDict_Fini()? _PyUnicode_Fini() calling PyDict_Clear(interned) to relase interned(no other malloc operation).
I am not sure it's ok or not.

_PyFloat_Fini(tstate);
_PyLong_Fini(tstate);
}
Expand Down