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.

Sign up for GitHub

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

Jump to bottom

Fuzzing MVP #149

Merged
merged 11 commits into from
Sep 23, 2020
Merged

Fuzzing MVP #149

merged 11 commits into from
Sep 23, 2020

Conversation

mingxguo27
Copy link
Contributor

MVP of OpenSK HID fuzzing with dependencies and linking errors fixed.

@google-cla google-cla bot added the cla: yes label Sep 16, 2020
kaczmarczyck
kaczmarczyck reviewed Sep 16, 2020
View reviewed changes
fuzz/fuzz_targets/fuzz_target_split_assemble.rs Outdated Show resolved Hide resolved
fuzz/fuzz_targets/fuzz_target_split_assemble.rs Show resolved Hide resolved
fuzz/fuzz_targets/fuzz_target_split_assemble.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
kaczmarczyck
kaczmarczyck previously approved these changes Sep 16, 2020
View reviewed changes
Copy link
Collaborator

@kaczmarczyck kaczmarczyck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for one more reviewer to do a pass.

ia0
ia0 reviewed Sep 16, 2020
View reviewed changes
fuzz/Cargo.toml Outdated Show resolved Hide resolved
fuzz/Cargo.toml Outdated Show resolved Hide resolved
fuzz/fuzz_targets/fuzz_target_split_assemble.rs Outdated Show resolved Hide resolved
fuzz/fuzz_targets/fuzz_target_split_assemble.rs Outdated Show resolved Hide resolved
third_party/lang-items/Cargo.toml Outdated Show resolved Hide resolved
ia0
ia0 previously approved these changes Sep 17, 2020
View reviewed changes
kaczmarczyck
kaczmarczyck previously approved these changes Sep 17, 2020
View reviewed changes
@mingxguo27 mingxguo27 dismissed stale reviews from kaczmarczyck and ia0 via 7517e6a September 18, 2020 11:49
kaczmarczyck
kaczmarczyck previously approved these changes Sep 18, 2020
View reviewed changes
@kaczmarczyck
Copy link
Collaborator

@jmichelp The binary seems to not be reproducible anymore. Is this related to this pull request? I'd propose to squash and merge on this one, and investigate later?

@jmichelp
Copy link
Collaborator

Should be fixed by #153
Please rebase

kaczmarczyck
kaczmarczyck previously approved these changes Sep 18, 2020
View reviewed changes
Copy link
Collaborator

@kaczmarczyck kaczmarczyck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give @gendx a chance to take a look next week. Otherwise LGTM.

gendx
gendx requested changes Sep 21, 2020
View reviewed changes
src/ctap/storage.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
third_party/lang-items/Cargo.toml Outdated Show resolved Hide resolved
@gendx
Copy link
Collaborator

gendx commented Sep 21, 2020

I'm concerned that removing the alloc_init feature from libtock-rs core breaks OpenSK, as no allocator is initialized.

This was referenced Sep 22, 2020
Merged
Closed
gendx
gendx reviewed Sep 22, 2020
View reviewed changes
Copy link
Collaborator

@gendx gendx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that #156 is merged, the changes in src/ and third_party/ shouldn't be needed anymore, and the fuzzing feature can be removed. The std and ram_storage features should be sufficient.

Note: cargo fuzz requires cargo install cargo-fuzz. This should be added to either setup.sh or a fuzz-setup.sh script.

fuzz/Cargo.toml Outdated
Comment on lines 17 to 20

[dependencies.ctap2]
path = ".."
features = ['std', 'ram_storage', 'fuzzing']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent with the other imports. It would be clearer to add a line in [dependencies]:

ctap2 = { path = "..", features = [ ... ] }

kaczmarczyck
kaczmarczyck previously approved these changes Sep 23, 2020
View reviewed changes
gendx
gendx reviewed Sep 23, 2020
View reviewed changes
Copy link
Collaborator

@gendx gendx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to add a GitHub workflow that does cargo fuzz build to make sure this doesn't break in the future.

@gendx gendx mentioned this pull request Sep 23, 2020
Merged
2 tasks
@gendx
Copy link
Collaborator

gendx commented Sep 23, 2020

No questions left from me. I'll leave it to @kaczmarczyck to merge this pull request.

@kaczmarczyck kaczmarczyck merged commit 55c9053 into google:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gendx gendx gendx approved these changes

@ia0 ia0 ia0 left review comments

@kaczmarczyck kaczmarczyck kaczmarczyck left review comments

@jmichelp jmichelp Awaiting requested review from jmichelp

@jennypullman jennypullman Awaiting requested review from jennypullman

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mingxguo27 @kaczmarczyck @jmichelp @gendx @ia0