gh-142665: fix UAF when accessing a memoryview concurrently mutates the underlying buffer#143324
gh-142665: fix UAF when accessing a memoryview concurrently mutates the underlying buffer#143324picnixz wants to merge 1 commit intopython:mainfrom
memoryview concurrently mutates the underlying buffer#143324Conversation
…utates the underlying buffer
| return NULL; | ||
|
|
||
| if (init_slice(&sliced->view, key, 0) < 0) { | ||
| self->exports++; |
There was a problem hiding this comment.
Can you add a comment explaining the purpose of the exports++? Add a reference to the GitHub issue (gh-142665:).
There was a problem hiding this comment.
Ah, I think I wasn't consistent in my reviews or in my commits. I thought the commit history would have been enough for that. But sure I can a comment
| if (init_slice(&sliced->view, key, 0) < 0) { | ||
| self->exports++; | ||
| int rc = init_slice(&sliced->view, key, 0); | ||
| self->exports--; |
There was a problem hiding this comment.
If we go this way, should not we call init_len() and init_flags() while export is incremented?
Also, we can have a similar issue with memory_item_multi().
There was a problem hiding this comment.
Also, memory_ass_sub() can have the same issue.
There was a problem hiding this comment.
For ass_sub, there is:
/* rvalue must be an exporter */
if (PyObject_GetBuffer(value, &src, PyBUF_FULL_RO) < 0)
return ret;so it's already protected I think
There was a problem hiding this comment.
But I wasn't aware of memory_item_multi which uses ptr_from_tuple and this looks like vulnerable.
There was a problem hiding this comment.
If we go this way, should not we call init_len() and init_flags() while export is incremented?
Strictly speaking no because those functions don't call any Python code, unless there are some invariants I'm not aware of
There was a problem hiding this comment.
PyObject_GetBuffer() is called for source, not for destination. Could not the destination be released while we calculate a slice?
There was a problem hiding this comment.
Hum. I actually tried this one. Maybe. I won't be much available for the next two weeks though so this will need to wait a bit!
This fix actually changes the existing behavior but messing up with the memoryview while still being accessed is honestly a poor design choice and I don't know when it'd be legitimate to support bad slicing.
memoryviewslicing via re-entrant__index__#142665