Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fuzzing fixes #147

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Fuzzing fixes #147

merged 3 commits into from
Feb 23, 2024

Conversation

Grinkers
Copy link
Collaborator

Cleaned up/rebased version of #125.

This PR is mostly for the tests and history, the actual parsing implementation (clone version) will be replaced in a future PR with the EdnError/parse rework as seen in Grinkers#6. I can squash it all, but I think this will make things easier to review.

@Grinkers Grinkers marked this pull request as ready for review February 12, 2024 21:47
@naomijub
Copy link
Owner

Why not check the exact error?

@Grinkers
Copy link
Collaborator Author

Why not check the exact error?

For what?

Generally speaking, this is coming up next. I'm just trying to keep the PR sizes down, but the EdnError rework is pretty big
Grinkers#6 (see the "Debug" section)
https://github.com/Grinkers/edn-rs/blob/4365f555f997df08ecffce32a65c18b0dd76870d/tests/error_messages.rs
https://github.com/Grinkers/edn-rs/blob/4365f555f997df08ecffce32a65c18b0dd76870d/src/edn/error.rs

EdnError won't be string based. It also won't be possible for user/test code to be able to create an arbitrary EdnError. Nor does it have added requirements like Eq or heap allocation.

More testing can be added to the future error_messages.rs. The current tests are useful for finding regressions or parsing issues, but they're not really useful for testing inconsistent/underspecified string-based messages.

After EdnError rework, I also had some WIP instrumentation coverage testing to get 100% coverage for messages, which is where these should be tested/specified
Grinkers@eac1f8b#diff-efdbad326cffce82491ae4d979ca521416f195633dca11d3960eefaed5415a0d

@Grinkers Grinkers force-pushed the fuzzing-fixes branch 2 times, most recently from 122eeaf to 4d2eabc Compare February 13, 2024 01:45
@Grinkers
Copy link
Collaborator Author

Rebased and just removed the commit dealing with tests, just to get things untangled.

@Grinkers Grinkers merged commit 95abc3f into naomijub:main Feb 23, 2024
14 checks passed
@Grinkers Grinkers deleted the fuzzing-fixes branch February 23, 2024 18:56
@Grinkers
Copy link
Collaborator Author

@naomijub I think this is a good time for a 0.17.5. Bonus points if we can do #138 at the same time to test it. This fixes a lot of things without introducing breaking changes going forward (I'm assuming the following release will probably be 0.18.x, moving towards a 1.0 goal).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants