fix leak in _PyCode_Update #48
Conversation
537b227 to
0bc1f4c
Compare
Python/marshal.c
Outdated
| @@ -325,6 +325,7 @@ w_ref(PyObject *v, char *flag, WFILE *p) | |||
| if (p->version < 3 || p->hashtable == NULL) | |||
| return 0; /* not writing object references */ | |||
|
|
|||
| // TODO: is this enough? what if v is a list containing a code object? | |||
There was a problem hiding this comment.
I think it's enough. All we're trying to protect against is values in refs being code objects (because code objects in refs are used as placeholders). Putting the list in refs is fine.
| co->co_filename = NULL; | ||
| co->co_name = NULL; | ||
| co->co_qualname = NULL; | ||
|
|
There was a problem hiding this comment.
Hm, I changed my mind again. I think we should leave init_code() unchanged (storing values without looking) and in _PyCode_Update() we explicitly DECREF co_name, co_qualname and co_filename (because on entering there we know they are valid).
Moreover, in _PyCode_Update() we should actually preserve the original co_filename, and if it's different, overwrite the co_filename of any (newly created) dehydrated code objects, probably using the same logic as update_compiled_module() in import.c. In case you wonder why, the reason is to preserve the filename set at the toplevel by _imp.fix_co_filename() under some circumstances (called from _bootstrap_external.py) -- there's an obscure test in Lib/test/test_import/init.py (test_incorrect_code_name) that comes down to verifying that if a module is loaded from bytecode, the co_filename of all the loaded code objects is where it's loaded from, not whatever was marshaled into the file. The existing logic doesn't set this for code objects that are skipped by lazy loading (I already had to fix a crash there due to co_consts being NULL), and hydrating currently overwrites the fixed co_filename with the incorrect one read from the marshal data. (Arguably we should not serialize co_filename at all, and only set it when loading. But that's probably going to break some other test...)
There was a problem hiding this comment.
Yes, I saw that test but didn't understand it. I'll try this in a new PR. I think update_code_filenames needs to move from import.c to codeobject.c.
There was a problem hiding this comment.
I spent some quality time with both C and Python debuggers before I understood the feature that it tests. :/ The key is the _imp.fix… call in _bootstrap_external.py.
This reverts commit 0bc1f4c.
I updated only UNARY_POSITIVE_R to the new format.
- Syntax `register inst(NAME, (ieffects -- oeffects)) { ... }`
- Map stack effects from/to `REG(opargN)` instead of PEEK()
- Use `Py_XSETREF(REG(opargN), result)` for output effects
- Suppress stack adjustment in epilogue
- Suppress effect of `DECREF_INPUTS()`
- Always go to plain `error` in `ERROR_IF()`
No description provided.