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
use after free in key sharing dict #73624
Comments
I've managed to create a minimal test file that causes python to crashe when run with python 3.6.0 (packaged in my distribution or from hg repository), 3.6 branch from hg repository and 3.7 branch from hg repository, but works fine on 3.5.3 and 3.5 branch. It also does not crash when python is compiled with '--with-pydebug' option (3.6/3.7) $ uname -a
Linux 4.8.13-1-ARCH #1 SMP PREEMPT Fri Dec 9 07:24:34 CET 2016 x86_64 GNU/Linux
$ python --version
Python 3.6.0
$ pip freeze
appdirs==1.4.0
packaging==16.8
pyparsing==2.1.10
python-dateutil==2.6.0
six==1.10.0
SQLAlchemy==1.1.5 To reproduce, use the attached file (minimal_crash.py): $ virtualenv3 venv
$ source venv/bin/activate
$ pip install sqlalchemy Run once to create DB and insert some stuff into it (no crash): Then re-run same thing WITHOUT re-creating the base: Runing with GDB: https://gist.github.com/audricschiltknecht/5564034c5aac78d881e03f29e069a8f5 |
I could reproduce this using Audric's command. And yes, debug build doesn't suffer from this crash. Nosy Victor. DEBUG:root:Called with: ['/tmp/minimal_crash.py', '-d', 'sqlite:///crash.db', '-v'] Program received signal SIGSEGV, Segmentation fault. ... |
valgrind output is here. It seems another bug relating to key-sharing dict... |
4385 int was_shared = cached == ((PyDictObject *)dict)->ma_keys; L4402 accessed free So right way to fix it may be DK_INCREF() before PyDict_SetItem(). |
You may try to reproduce with a release build and PYTHONMALLOC=debug |
This is use after free, not overflow. |
(PYTHONMALLOC=malloc valgrind find it soon. :) |
Please update the issue title. |
I can reproduce it on Python 3.5 with attached script. "Trigger key sharing dict resize while callbacks (weakref or __del__) called from setitem" is step to reproduce. Should I fix it for 3.3~ (security fix only) or 3.5~ (bugfix)? |
Is it related to bpo-27945? |
It's similar to bpo-27945, but different. I confirmed this issue is in 3.4 too. // _PyObjectDict_SetItem()
if ((tp->tp_flags & Py_TPFLAGS_HEAPTYPE) && (cached = CACHED_KEYS(tp))) {
...
res = PyDict_DelItem(dict, key); // <- may run arbitrary code. CACHED_KEYS(tp) can be changed and cached can be freed here.
if (cached != ((PyDictObject *)dict)->ma_keys) {
CACHED_KEYS(tp) = NULL;
DK_DECREF(cached); // !!!
}
} else {
res = PyDict_SetItem(dict, key, value); // Same to DelItem().
if (cached != ((PyDictObject *)dict)->ma_keys) {
/* Either update tp->ht_cached_keys or delete it */
if (cached->dk_refcnt == 1) { // !!! |
PyDict_SetItem() can trigger destructor which first call _PyObjectDict_SetItem() which change CACHED_KEYS(tp) and then call PyDict_SetItem() which call dictresize(). At the end it may be possible that cached != ((PyDictObject *)dict)->ma_keys and cached != CACHED_KEYS(tp) and CACHED_KEYS(tp) != ((PyDictObject *)dict)->ma_keys. Wouldn't be better to just update the cached variable after calling PyDict_SetItem()?
|
+1 on this and I think the deletion should also use if ((cached = CACHED_KEYS(tp) != NULL) |
I left one review about the comment on Rietvied last patch. :-) |
Could you please write tests Inada? It would be nice to test also the case that fails with 29438-sharedkey-useafterfree-py36.patch. Actually I don't know if this is easy to reproduce, it was just my guessing. |
to: Serhiy I can reproduce the issue by 29438-minimum.py, on Python 3.7 with pydebug. |
Okay, if there is no way to test this with certainty, tests may be omitted. Why res == 0 is added? If PyDict_SetItem() triggers recursive calling of _PyObjectDict_SetItem() which calls PyDict_SetItem() it may be possible that the first PyDict_SetItem() is failed while the dict is changed by the second PyDict_SetItem() and CACHED_KEYS(tp) becomes outdated. |
To avoid hiding error raised in PyDict_SetItem(). BTW, how about -py35.patch? It is minimum patch to avoid "use after free". It skip |
I think same patch should be applied to Python 3.5 too. |
Since Python 3.5's key sharing dict allows deletion, py35-2.patch is slightly different from py36-2.patch. Since dictresize won't happen in normal (no weakref/del callback) deletion, |
I'll send PR to github. Please continue there. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: