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

Implement fuzzer for normalize #81

Merged
merged 4 commits into from
Sep 16, 2023
Merged

Implement fuzzer for normalize #81

merged 4 commits into from
Sep 16, 2023

Conversation

kdarkhan
Copy link
Collaborator

@kdarkhan kdarkhan commented Sep 15, 2023

This addresses #57.

I am focusing on mdbook-i18n-normalize binary only in this PR. I will implement fuzzers for other binaries separately.
In order to fuzz the logic, I had to move it out of binary to library. The first commit
copies the binary file without making any changes to make the review easier.

I confirmed that when running the new fuzzer against the version of the code
before #56 was merged,
it finds a failure pretty quickly.

@mgeisler
Copy link
Collaborator

In order to fuzz the logic, I had to move it out of binary to library. The first commit
copies the binary file without making any changes to make the review easier.

Nice, thanks for thinking of your reviewer! 🙂

Please squash the first two commits into one so that you move the code instead of copying and deleting it. The way I see it, the repository should look good at any commit and ir someone where to checkout the first commit, they would probably wonder about the duplicate code

Also, please name the new module nornalize, I think the i18n_ prefix is redundant since we're already inside a mdbook_i18n_helpers module.

This refactoring is needed so that fuzzing code can
import and test this logic.
I tested this fuzz target with changes from
google#56 reverted
and it does detect the panic.
@kdarkhan kdarkhan changed the title Implement i18n_normalize fuzzer Implement fuzzer for normalize Sep 16, 2023
@kdarkhan
Copy link
Collaborator Author

Sounds good. I removed the prefix i18n_ from the module name and the fuzz target name, squashed the commits as well.

src/normalize.rs Outdated Show resolved Hide resolved
@mgeisler mgeisler enabled auto-merge September 16, 2023 14:27
@mgeisler
Copy link
Collaborator

Awesome, this looks great!

@mgeisler mgeisler merged commit 742a6f2 into google:main Sep 16, 2023
@mgeisler
Copy link
Collaborator

@kdarkhan, would #83 be something for you since you're already looking at fuzzers?

@kdarkhan
Copy link
Collaborator Author

@mgeisler, sure, I will work #83 as well once I am done with the other 2 fuzz tests.

@mgeisler
Copy link
Collaborator

Thanks!

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