search

mimblewimble / rust-secp256k1-zkp

ZKP fork for rust-secp256k1, adds wrappers for range proofs, pedersen commitments, etc
Creative Commons Zero v1.0 Universal
56 stars 51 forks source link

Fix RangeProof::clone() memory corruption and remove rustc-serialize dependency #83

Closed scilio closed 3 months ago

scilio commented 3 months ago

The previous code was copying beyond the buffer, which resulted in memory corruption. I change it from copying mem::size_of::<RangeProof>() bytes, which included the additional plen field, to just self.plen bytes.

I also removed the rustc-serialize dependency, which prevents us from building with newer rust versions.

The tests all pass for me with these changes.

yeastplume commented 3 months ago

Thank you for looking into this, I just want to take a bit of time to understand the changes, and particularly this apparent memcpy bug (and why it's gone unnoticed for so long). I think it's because it's not actually being called from anywhere in Grin, but I need to make sure.

Do you have an example of a test somewhere that calls RangeProof::Clone and causes failure?

yeastplume commented 3 months ago

a RangeProof field is included in the Output type grin core's core/transaction.rs, and while we don't call RangeProof::clone() explicitly anywhere, some functions that operate on vectors of them, such as to_vec() and extend_from_slice() require the Clone trait to be implemented for T, however given that Clone is a supertrait of Copy they're usually both required, and a function requiring the Clone trait doesn't necessarily mean Clone is called if a copy suffices.

Also, I don't know why someone implemented an explicit Clone function here, surely it can just be auto-derived?

yeastplume commented 3 months ago

I have been running and testing for a while, both the wallet and node, with a modded version of the clone function that simply panics when called. So far so good, so evidence is pointing to this thankfully not being an issue in the current code. I think the clone method can safely be derived though, rather than having a custom implementation in there.