Fix Use-After-Free in NumPy/df #478
Conversation
ax3l
added
bug
For AoS and SoA `.to_numpy()` and dependent logic like `.to_df()`, the lifetime of the temporary copied variable is only handled by NumPy if we first create a named variable. The `to_host()` returned object is a temporary, and `np.array` using the `__array_interface__` protocol does not keep it alive automatically unless it is stored in an actual variable (eg., `tmp`).
ax3l
force-pushed
the
fix-to-host-temporary
branch
from
a7dfdc0 to
da9d3a4
Compare
| import numpy as np | ||
|
|
||
| if self.size() > 0: | ||
| if copy: |
There was a problem hiding this comment.
We can generally make this a bit more explicit by checking if self is on GPU and otherwise just copying from it directly, with another if/else branch. But the logic here should have the same effect and amount of copies involved...
The only difference is that here we will always end up with pinned memory, while we might be sometimes interested using regular host memory. But seems niche.
roelof-groenewald
left a comment
roelof-groenewald
left a comment
There was a problem hiding this comment.
Looks good to me.
Just one question: Do we need the asserts to "trick" Python into keeping the object alive?
|
The assert is not needed, but if it is ever violated I want a python error instead of a segfault in code that uses the returned variable. I was too lasy to |
|
Ah, |
| # np.array using the __array_interface__ protocol does | ||
| # not keep it alive automatically unless it is stored | ||
| # in an actual variable (tmp). | ||
| tmp = self.to_host() |
There was a problem hiding this comment.
Hm, looks like there is more / this is not a sufficient fix (at least for some SP tests on conda-forge):
conda-forge/impactx-feedstock#56 (comment)
It already has an issue here, so the problem could be in the implementation of to_host()...
https://github.com/AMReX-Codes/pyamrex/blob/25.09/src/Base/PODVector.cpp#L79-L87
There was a problem hiding this comment.
Very hard to reproduce locally... also cannot spot anything in valgrind yet.
There was a problem hiding this comment.
seems to be fixed by BLAST-ImpactX/impactx#1156 o.0
2 participants
For AoS and SoA
.to_numpy()and dependent logiclike
.to_df(), the lifetime of the temporary copied variable is only handled by NumPy if we first create a named variable.The
to_host()returned object is a temporary, andnp.arrayusing the__array_interface__protocol does not keep it alive automatically unless it is stored in an actual variable (eg.,tmp).X-ref: