FluidityProject / fluidity

Fluidity
http://fluidity-project.org
Other
365 stars 115 forks source link

Allocate empty attribute arrays for detectors #319

Closed angus-g closed 3 years ago

angus-g commented 3 years ago

When we allocate/unpack detectors from Zoltan, some code expects that their attribute arrays (used for particles) are at least allocated, even if length 0. Ensuring this is done avoids potential segfaults when these arrays are unconditionally accessed.

Thanks @stephankramer for the detailed analysis of this issue! I think this is probably the right place for this allocation? Of course, the other approach would be to figure out where attribute_size is being passed into unpack_detector as a non-optional on a detector. (As an aside, that subroutine looks like it could just take a non-optional attribute_size and then the logic wouldn't be exactly duplicated in both branches...)

Closes #317.

angus-g commented 3 years ago

Hmm, not sure why tests are failing in the Docker configuration here. Is it because I opened the PR against a branch on my own fork?

Patol75 commented 3 years ago

Answer here I believe: https://github.com/FluidityProject/fluidity/pull/303#issuecomment-836654951

tmbgreaves commented 3 years ago

@angus-g - yes, sorry about that - I've not yet figured out a clean way to fix PRs from outside the main repo. The problem (I think) is that it's indeterminate where to pull docker secrets from, or possibly a security issue for remote forks to be able to use secrets from the Fluidity repo, so the docker login fails. We tried replicating the secrets in the PR source repo but that didn't work either.

Might you be able to push your branch to the main Fluidity repo and PR from that please?

tmbgreaves commented 3 years ago

Thank you very much for the fix here! Additionally, I think (from @drhodrid 's comment) that @Cmath2 is no longer working on Fluidity; might you be able to request a different reviewer when reopening the PR please?