Safe version #1
Merged
Safe version #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cardinality estimator makes assumptions about the values it receives when deserializing which are upheld by _`serde_json`_ but not `serde` in general, leading to buffer overflows and other problems when used with other serde de/serializers. I had the wildest crashes happening in a project, which I traced to being at least _related_ to cardinality-estimator, since they stopped when I shimmed it out. In narrowing down a smaller reproducible example, I found that using bincode with this library was sufficient to get buffer overflows: see https://github.com/uniphil/whose-overflow-is-it-anyway This PR adds a fuzz target for `postcard`, whose API is close enough to `serde_json` for the change from the serde fuzz target to be a single line. It also adds postcard calls to the normal serde tests, though I wasn't able to specifically write a failing regression test case for it (I'm sure it's possible, just had to stop somewhere). To be clear: this change only illustrates the issue and does not include a fix. Seems like fixing will require either - fixing the deserialization logic to assume less about the inputs it receives (array alignment and length?), OR - documenting explicitly that the `serde` support is **only** safe to work with `serde_json`. Thanks @DavidBuchanan314 for help narrowing the source of the crashes <3
these ones are probably fine, probably just giving up perf here...
i guess it's usize because a pointer is masked out from it... but imo it's really more about the bytes in more cases, and turning it into a pointer is a weird special case.
|
✅ custom serde: gone |
serde_tuple didn't like the const generics (or just the generics?) anyway for compact rep probably use bincode
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
removes all uses of unsafe. even the ones that are probably fine. also loses the specifically vectorized checks on insert in array form... could be brought back but i don't need that level of micro-optimization to bother getting it to work again.
the serde stuff has a redundant tuple wrapping everything that i need to remove. it's now using serde-derive, but i want to pass through from the public API to the representation directly. ORRRrrrr collapse the estimator to impl directly on the representation and then maybe no custom serde is needed at all.
i'll probably also add bincode derives (even more reason to collapse i guess)
going to roll ahead with this for UFOs since i can't wait for cloudflare/cardinality-estimator#12 any more. the serialized sketches likely won't be directly compatible, so this fork might just keep existing.