search

mimblewimble / secp256k1-zkp

Fork of secp256k1-zkp for the Grin/MimbleWimble project
MIT License
32 stars 42 forks source link

Fix the expected allocation count which would fail allocation on 32-bit systems #38

Closed jasonxh closed 5 years ago

jasonxh commented 5 years ago

When running Grin server on Raspberry Pi 3, this issue manifests itself as a segfault due to allocation failure (mentioned here https://github.com/mimblewimble/grin/issues/1681#issuecomment-447974380).

The reason is we made 3 allocations (when n_proofs == 1) right afterwards but only reserved 2 alignment slots. On 64-bit system, all 3 allocations happen to fit in 2 alignment slots so we are fine, but on 32-bit system we are 4 bytes short and the last allocation request would fail.

garyyu commented 5 years ago

@jasonxh thanks for your PR.

The reason is we made 3 allocations (when n_proofs == 1) right afterwards but only reserved 2 alignment slots.

I'm not sure I understand well on your root cause analysis, what's the meaning of this? could you please explain more? Where's that 3 allocations and where is the only reserved 2 alignment slots?

jasonxh commented 5 years ago

@garyyu each object allocation is made by calling secp256k1_scratch_alloc (line 148, 149, and 152). It's a total of 2 + n_proofs in this scratch frame. When allocating a scratch frame (secp256k1_scratch_allocate_frame), the caller needs to specify both the total amount of memory needed, as well as the number of object allocations that are going to happen in this frame.

The latter is needed due to alignment (hard coded at https://github.com/mimblewimble/secp256k1-zkp/blob/master/src/scratch_impl.h#L16). As a result of alignment, each object allocation may end up taking some extra bytes ([0, ALIGNMENT - 1]). Thus secp256k1_scratch_allocate_frame needs to take that into account to allow some "wiggle room".

garyyu commented 5 years ago

each object allocation is made by calling secp256k1_scratch_alloc (line 148, 149, and 152). It's a total of 2 + n_proofs in this scratch frame.

Thanks @jasonxh to track this down 👍 It's a really a bug here, even on 64-bit system it's risky to fail when n_proofs is a random value other than 1 or 1000.

garyyu commented 5 years ago

And I will report this bug to the upstream repo for you :-)

0xmichalis commented 5 years ago

@garyyu can this be merged now? Seems Andrew fixed it in his PR.

garyyu commented 5 years ago

Ok, anyone else want a review on this? if not, I can merge it and make a new tag on rust-secp256k1-zkp repo.