search

indexmap-rs / indexmap

A hash table with consistent order and fast iteration; access items by key or sequence index
https://docs.rs/indexmap/
Other
1.71k stars 150 forks source link

Add `borsh` serialization support #313

Closed sug0 closed 8 months ago

sug0 commented 8 months ago

Add support to serialize maps and sets with borsh.

sug0 commented 8 months ago

@cuviper the new deserialization code in spite of working correctly, introduced some exploits related with OOM errors. I'm inclined to rollback to Vec's borsh deserialization code, considering the gains in performance of the new approach are very minute anyway, and their deserialization code already solves the problem at hand. lmk what you think

cuviper commented 8 months ago

introduced some exploits related with OOM errors.

Please explain this! If there's an "exploit" footgun here, it needs to be commented at the very least.

sug0 commented 8 months ago

@cuviper hm the problem is this deserialization code I wrote is blindly allocating a buffer for incoming items from a stream. the deserialization code in borsh for collections already guards against OOM related issues by limiting the size of allocations (possibly why it's occasionally slower in the benches I ran).

I'll revert the deserialization code to use Vec::deserialize_from_reader as I had before

cuviper commented 8 months ago

Oh, I see -- I should really add limits to serde size_hint() deserializers as well, as they do in their own impls.

cuviper commented 8 months ago

Your branch is not open to maintainer pushes, so I put suggested changes in my fork: https://github.com/heliaxdev/indexmap/compare/heliax/borsh-support...cuviper:indexmap:borsh-support

sug0 commented 8 months ago

@cuviper thanks for the patches! I've included them in this branch

sug0 commented 8 months ago

rebased on 2.2.4

cuviper commented 8 months ago

This has been published in 2.2.5!

sug0 commented 8 months ago

sweet. thanks!