Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Add util/mem to zero out memory on drop. #8356

Merged
merged 5 commits into from
Apr 11, 2018
Merged

Add util/mem to zero out memory on drop. #8356

merged 5 commits into from
Apr 11, 2018

Conversation

twittner
Copy link
Contributor

No description provided.

@parity-cla-bot
Copy link

It looks like @twittner signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@twittner twittner added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Apr 10, 2018
}
}

impl AsMut<[u8]> for H256Mut {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is lack of this impl the only reason you created H256Mut? I see no reason why it shouldn't be added to paritytech/primitives. Please make a pr with this change to that repo :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH I don't see if it fits primitives. All primitives we have are immutable, do we want to have both mutable and immutable ones there? IMHO makes the library unnecessary bloated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already DerefMut in primitives though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, ok. I thought we want to introduce H256Mut struct into that repo. If it's only about adding missing AsMut trait implementation then it's all good!

@debris debris added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 10, 2018

/// Wrapper to zero out memory when dropped.
#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct Memzero<T: AsMut<[u8]>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not replace the content in place and just require that T: Default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand. Could you explain this a more fully please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nvm, actually it's better with AsMut if we have all the supported structs.

@5chdn 5chdn added this to the 1.11 milestone Apr 10, 2018

for topic in topics {
buf.extend(&*(topic ^ key));
}

let _ = Memzero::from(key.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't that just copy the value of key into some other place in the stack and then zero that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks.

@twittner twittner added the A0-pleasereview 🤓 Pull request needs code review. label Apr 11, 2018
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 11, 2018
@debris debris merged commit 2b05eb4 into master Apr 11, 2018
@debris debris deleted the zero-on-drop branch April 11, 2018 11:57
@5chdn 5chdn removed the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Apr 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants