Make the memory soft cap configurable and make Settings non-exhaustive

[koute]

This is the continuation of #6

[ordian]

Looks good. Regarding #6 (comment), I think it's possible to change

ws-rs/src/circular_buffer.rs

Line 144 in 401d8c1

if self.remaining() > limit || self.current_capacity() <= limit {

to

if self.remaining() > limit / 2 || self.current_capacity() < 2 * limit {

and then it would safe to apply in all cases, not just when self.is_empty().

[koute]

Looks good. Regarding #6 (comment), I think it's possible to change

ws-rs/src/circular_buffer.rs

Line 144 in 401d8c1

if self.remaining() > limit || self.current_capacity() <= limit {

to

if self.remaining() > limit / 2 || self.current_capacity() < 2 * limit {

and then it would safe to apply in all cases, not just when self.is_empty().

Hmm... so only shrink it if the amount of data remaining in the buffer is at most half of the limit and the current capacity is at least double the limit? But couldn't you still do something like this then?

1. Write limit * 0.5 bytes. Assume the capacity is also limit * 0.5 bytes big.
2. Write 2 bytes. The buffer contains limit * 0.5 + 2 bytes of data. Capacity grows to limit * 2. The second condition is fulfilled.
3. Read 2 bytes. The buffer contains limit * 0.5 bytes. The first condition is fulfilled.
4. The buffer is reallocated. limit * 0.5 bytes is copied to the new buffer at the cost of sending only 2 bytes by the attacker. The new capacity is again limit * 0.5 bytes.
5. Go to (2).

Did I get that right?

(Of course whenever this is actually feasible is another matter altogether, since there would be a need to maintain a constant limit * 0.5 bytes of data in the buffer, and we're the ones doing the reads, not the attacker.)

A possible fix for this would be to change the second condition to require at least double the limit plus 1 of capacity, I think? (So change < 2 * limit to <= 2 * limit.)

Also, the second condition would prevent the buffer to be freed even when the buffer is empty and it's over the soft cap but still under the double of the soft cap (so the soft cap would effectively become a half soft cap then). I think special-casing this when the buffer is empty so that it continues to still work as it does now would be a good idea.

[ordian]

The buffer contains limit * 0.5 + 2 bytes of data. Capacity grows to limit * 2

Why does capacity grow to 2 * limit in this case? It grows that large when there is at least limit of data. The idea here is that the user will have to write/read the amount of bytes proportional to the reallocation.

I've done amortized analysis on this during my university times. Basically, if you want to implement shrinking operation on a Vec, you should shrink capacity by 2 when capacity / size >= 4. This guarantees O(1) amortized complexity in combination with 2x capacity growth.

[koute]

The buffer contains limit * 0.5 + 2 bytes of data. Capacity grows to limit * 2

Why does capacity grow to 2 * limit in this case?

Ah, sorry, yeah, you're right of course. That was a brainfart on my part. I had a feeling I was probably wrong, considering how late it is here. (:

[ordian]

No worries :) Glad we're on the same page now. Feel free to merge and publish w/ or w/o the suggested improvement.

[koute]

@ordian Okay, I've added a new commit with your suggestion! It'd be great if you could take one final look at it. (:

[ordian]

LGTM, thanks!

[koute]

Thanks!

I'd be great if you could tag and release a new version when you have a moment. (I'm not sure whenever I can do it myself; probably not?) I'll take care of bumping this in polkadot/substrate later.

[ordian]

I'd be great if you could tag and release a new version when you have a moment.

Published.

I'm not sure whenever I can do it myself; probably not?

Once you're added to the https://github.com/orgs/paritytech/teams/core-devs/members.

I'll take care of bumping this in polkadot/substrate later.

That might be not so easy, because substrate still uses jsonrpc v15. But there is no rush.