Skip to content
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

Disallow pickle when storing numpy array in ArrayData #3434

Merged
merged 3 commits into from
Oct 18, 2019

Conversation

greschd
Copy link
Member

@greschd greschd commented Oct 17, 2019

Following the discussion in #3411.

Unpickling untrusted data is a potential security risk, since it
allows arbitrary code execution. For this reason, numpy changed
its 'load' method to no longer allow pickles by default. This
means that in AiiDA, ArrayData containing pickles can be stored,
but not loaded. To make things consistent, we now also disallow
pickle when saving the array. Besides the security risk, pickle
is also problematic in the context of AiiDA because it can also
become unreadable e.g. when the classes it contains change names.
@greschd greschd requested a review from sphuber October 17, 2019 13:10
@greschd
Copy link
Member Author

greschd commented Oct 17, 2019

We could also add allow_pickle=False on the load method: In principle it's not necessary because the numpy version required by aiida-core does that already, but it's possible some people have an older version installed for some reason.

@sphuber
Copy link
Contributor

sphuber commented Oct 17, 2019

If we decide that this is always the behavior we want, I would vote for also putting it explicitly on the load method, just in case numpy ever decide to change the default (not very likely, but still).

@greschd
Copy link
Member Author

greschd commented Oct 17, 2019

Ok, I updated it accordingly.

@sphuber
Copy link
Contributor

sphuber commented Oct 18, 2019

@giovannipizzi I would be OK with this change with the idea "better safe than sorry".

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I actually think we should not use pickles and this is the right thing to do. Thanks!

@sphuber sphuber merged commit 26a671f into aiidateam:develop Oct 18, 2019
@sphuber
Copy link
Contributor

sphuber commented Oct 18, 2019

Thanks a lot @greschd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants