naomijub / edn-rs

[DEPRECATED]: Crate to parse and emit EDN
https://crates.io/crates/edn-rs
MIT License
81 stars 14 forks source link

Various fixes for problems found while fuzzing valid EDN. #125

Closed Grinkers closed 9 months ago

Grinkers commented 9 months ago

The two big problems found were not handling characters correctly and failing to parse single digit numbers(!).

I've also implemented Arbitrary Numbers, but handled as strings. This way no data is ever lost, but this goes beyond the scope of std-only. In the future a feature could be added to change the type (or introduce a breaking change if rust std ever gets these features).

Display now more closely matches clojure too.

I've run a few million iterations with my fuzzer https://github.com/Grinkers/edn-rs_fuzzer (using clojure's EDN generator). I'll fuzz incorrect EDN after we decide on the new Error and I implement a walker.

codecov-commenter commented 9 months ago

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (ae44881) 70.63% compared to head (1d403d7) 72.99%. Report is 2 commits behind head on master.

Files Patch % Lines
src/edn/mod.rs 76.31% 9 Missing :warning:
src/edn/utils/index.rs 0.00% 2 Missing :warning:
src/json/mod.rs 0.00% 2 Missing :warning:
src/deserialize/parse.rs 95.83% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #125 +/- ## ========================================== + Coverage 70.63% 72.99% +2.36% ========================================== Files 8 8 Lines 841 874 +33 ========================================== + Hits 594 638 +44 + Misses 247 236 -11 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Grinkers commented 9 months ago

Sorry for all the force pushes, it's a bit annoying not being able to easily see the results locally. Make was just blowing past everything, so maybe need to come up with something better. I also confused myself as this is on my fork. I'll keep this PR as-is.

Grinkers commented 9 months ago

@naomijub https://github.com/naomijub/edn-rs/pull/125/commits/7969aad52dba7dea99529da5d430e6112714c80b is pretty critical.

Still need feedback on Arbitrary length things. Roundtrip compatible but otherwise not handled (future functions can be added to parse/convert), but the internal structure (String) is probably ok for now

naomijub commented 9 months ago

@naomijub

https://github.com/naomijub/edn-rs/pull/125/commits/7969aad52dba7dea99529da5d430e6112714c80b

is pretty critical.

Still need feedback on Arbitrary length things. Roundtrip compatible but otherwise not handled (future functions can be added to parse/convert), but the internal structure (String) is probably ok for now

I will try to review it this weekend

Grinkers commented 9 months ago

Conflicts from #122 are resolved with a rebase. No other changes intended.

naomijub commented 9 months ago

I'm actually good with the PR, just take a look at the idea of wrapping everything into a peekable. I would love if @evaporei could code review as well, as this is a larger PR

Grinkers commented 9 months ago

Bump. It'd be really good to get this reviewed. https://github.com/edn-rs/edn-rs/pull/125/commits/1942a7d81f3a07db115e5161e18a5af93c8ecb58 is a very critical bug fix.

I can break out the Arbitrary numbers if it that's the holdup.

Grinkers commented 9 months ago

Rebased against main.

I removed the arbitrary length int/floats. I'm not happy with it long-term, as I don't feel it's suitable for a 1.0 target. I moved that to https://github.com/edn-rs/edn-rs/pull/134 as a draft while I think about it. This PR as-is should be considered high priority due to not following the spec.

Grinkers commented 9 months ago

Rebased/conflicts resolved, again.

Grinkers commented 9 months ago

Would it help if I broke things up into more pieces so it's easier to review and track? @naomijub @evaporei

Grinkers commented 9 months ago

Rebased in https://github.com/Grinkers/edn-rs/pull/3 and part of #140