radicle-dev / radicle-link

The second iteration of the Radicle code collaboration protocol.
Other
423 stars 39 forks source link

protocol: Guard against underflow in promotion #659

Closed xla closed 3 years ago

xla commented 3 years ago

If the number of peers currently active in the view is larger than the configured max, the calculation to pick a random number of peers from the passive list will panic. If the subtraction on usize underflows it will return usize::MAX which in turn will cause trouble in the allocation code of raw vectors as it hits the max capacity branch in the alloc_guard. The reason this is happening in the first is that choose_mutliple of the rand crate creates a Vec with the given amount in our case n. See links below to decent into madness:

https://github.com/rust-random/rand/blob/30d2d98df5d09c524a30c8772fc7c8b203f5b70a/src/seq/mod.rs#L468 https://github.com/rust-lang/rust/blob/master/library/alloc/src/raw_vec.rs#L188 https://github.com/rust-lang/rust/blob/e5f83d24aee866a14753a7cedbb4e301dfe5bef5/library/alloc/src/raw_vec.rs#L198

Signed-off-by: Alexander Simmerl a.simmerl@gmail.com

alexjg commented 3 years ago

Should we log that this case has occurred? Does this situation indicate a misconfiguration somewhere?

xla commented 3 years ago

Should we log that this case has occurred? Does this situation indicate a misconfiguration somewhere?

I don't think it does, it is conceivable that even when at max that new peers can intermittently sneak into the active set and therefore we are in a situation where there is an off by one situation - @kim will know better for sure.

kim commented 3 years ago

This alters the meaning, though: suppose the promotion of all selected passive peers succeeds, it will replace the active view completely. I am not sure if this is a good thing.

The overflow could only occur because of a configuration error, so I'm not sure this is still necessary.

kim commented 3 years ago

I would probably just put an assert! there, as an invariant is broken. Or default to 1.