Python: fix load_chunk to temporary #913
Conversation
ax3l
force-pushed
the
topic-pyStoreChunkInMem
branch
2 times, most recently
from
4c7e3c0 to
aab4d89
Compare
|
Yes, that's the follow-up PR to address the additional issue you mentioned and add an overload to allow in-memory reads:
|
ax3l
force-pushed
the
topic-pyStoreChunkInMem
branch
3 times, most recently
from
129d261 to
512d8c2
Compare
| @@ -1636,6 +1636,11 @@ def writeFromTemporaryStore(self, E_x): | |||
| data = np.array([[1, 2, 3]], dtype=np.dtype("int")) | |||
| E_x.store_chunk(data) | |||
|
|
|||
| def loadToTemporaryStore(self, r_E_x): | |||
| # not catching the return value shall not result in a use-after-free: | |||
| r_E_x.load_chunk() | |||
There was a problem hiding this comment.
@franzpoeschel it looks like these tests does not trigger the issue we were concerned about.
Maybe the GC zeroes out the data it frees or we are otherwise lucky... But then we should see this throw: https://github.com/openPMD/openPMD-api/blob/0.13.1/include/openPMD/RecordComponent.hpp#L334-L335
Do you have another Python snippet we could add in the test here that demonstrates the load_chunk issue?
There was a problem hiding this comment.
I'd say this is likely a combination of (1) the rather small dataset that we test this on, (2) the fact that py::array initializes its data upon constructon and (3) that we don't allocate anything in between. So, maybe we could trigger this by making things go a bit wilder.
There was a problem hiding this comment.
https://github.com/franzpoeschel/openPMD-api/tree/topic-pyStoreChunkInMem-increaseBuffersize
This branch triggers the issue on my machine and your implementation fixes it.
There was a problem hiding this comment.
Great hint, thank you - increasing the tested memory triggers the issue reliably :)
ax3l
changed the title
ax3l
changed the title
@franzpoeschel sorry, there are too many LOC and feature editions in #901 that I did not see the partly overlap 🙈 I splitted my PR in a bugfix (this PR) and a feature addition (#914) to make them concise, backportable and easy to review. Let's see how we mingle them together. Is #901 getting smaller once its dependent PR is merged and it can be rebased? :) |
ax3l
force-pushed
the
topic-pyStoreChunkInMem
branch
from
dc7444f to
4c9f9fb
Compare
A bit, but not drastically. Since this PR is the more "precise" one, I'd say go forward with implementing things here. I'll deal with getting things compatible during rebasing afterwards. |
This should trigger a use-after-free.
ax3l
force-pushed
the
topic-pyStoreChunkInMem
branch
from
47a0200 to
4675b01
Compare
Fix a use-after-free with load_chunk if the user discards the returned object before flush.
* Python Test: Discard loaded chunk This should trigger a use-after-free. * Fix Python: discarded load_chunk Object Fix a use-after-free with load_chunk if the user discards the returned object before flush.
Keep a reference in
load_chunkuntil data is not used anymore.Follow-up to #912 and related to #833