Skip to content

Conversation

@c410-f3r
Copy link
Contributor

Also adds support for Clippy

Clippy

Has a bunch of useful security related lints that verifies poor handled code.

Fuzz tests

Verifies user inputs that could abort the runtime execution. Basically, anything that uses unwrap, expect, indexing or non-checked APIs like Vec::remove.
After running the targets for some time, no error was found.

@lsaether The error I talked earlier was a false-positive related to the maximum number of allowed threads, i.e., nothing related to our code-base.

@c410-f3r c410-f3r requested review from lsaether and sea212 May 16, 2021 16:17
@c410-f3r c410-f3r force-pushed the caio-clippy branch 2 times, most recently from 0595fee to 6e073ca Compare May 16, 2021 16:21
Copy link
Contributor

@sea212 sea212 left a comment

Choose a reason for hiding this comment

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

Looks good overall. The general code quality definitively has been improved, thanks.
Is the only way to execute tests now by testing each crate on its own (or using the supplied test script)? I get an error when executing cargo test --features mock:

Running unittests (target/debug/deps/full_workflow-ba1fab45be0e51f1)
WARNING: Failed to find function "__sanitizer_acquire_crash_state".
WARNING: Failed to find function "__sanitizer_print_stack_trace".
WARNING: Failed to find function "__sanitizer_set_death_callback".
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 4172032396
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
#2 INITED exec/s: 0 rss: 64Mb
ERROR: no interesting inputs were found. Is the code instrumented for coverage? Exiting.
error: test failed, to rerun pass '-p zrml-prediction-markets-fuzz --bin full_workflow'

Comment on lines +28 to +38
mock = [
"orml-currencies",
"orml-tokens",
"pallet-balances",
"pallet-timestamp",
"sp-api",
"zrml-prediction-markets-runtime-api"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really no other way to make fuzz use the mocks only when required? Feels very unnatural now to execute tests, because they demand --features mock (yet tests imply mocks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually possible and easier to write

[dev-dependencies]
zrml-prediction-markets = { features = ["mock"], path = "." }

and then simply type

cargo test

So let's do it! We just need to be careful to not introduce cargo dependencies when publishing. Some projects had trouble in this scenario. For example, rust-lang/futures-rs#2305.

);

let _ = PredictionMarkets::on_initialize(6);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Many PM pallet functions are still missing. Are you planning to add them in a follow up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This subject can be tackled in different ways:

  • One fuzzing target per module entry-point
  • Specific workflows (join and then exit pool; create market, buy pool shares and then resolve; etc)
  • Everything at once, i.e., call every entry-point in the same fuzzing target

So yeah, I am planning to research, add more targets that makes sense, extend to overbook-v1 and test possible internal mutations that could cause program abortion.
As soon as this PR is merged, I will open an issue to track everything

@c410-f3r
Copy link
Contributor Author

Looks good overall. The general code quality definitively has been improved, thanks.
Is the only way to execute tests now by testing each crate on its own (or using the supplied test script)? I get an error when executing cargo test --features mock:

Running unittests (target/debug/deps/full_workflow-ba1fab45be0e51f1)
WARNING: Failed to find function "__sanitizer_acquire_crash_state".
WARNING: Failed to find function "__sanitizer_print_stack_trace".
WARNING: Failed to find function "__sanitizer_set_death_callback".
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 4172032396
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
#2 INITED exec/s: 0 rss: 64Mb
ERROR: no interesting inputs were found. Is the code instrumented for coverage? Exiting.
error: test failed, to rerun pass '-p zrml-prediction-markets-fuzz --bin full_workflow'

Sorry, didn't test cargo test in the root directory. This issue is now fixed

@c410-f3r c410-f3r added the s:review-needed The pull request requires reviews label May 25, 2021
Copy link
Member

@lsaether lsaether left a comment

Choose a reason for hiding this comment

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

This is great! It will improve our security and help us confidently build a safer runtime over time. Overall the code quality has been improved. LGTM

@lsaether lsaether added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels May 25, 2021
@c410-f3r
Copy link
Contributor Author

Thanks @lsaether
All concerns of @sea212 were addressed so I will merge

@c410-f3r c410-f3r merged commit aee304b into main May 25, 2021
@c410-f3r c410-f3r deleted the caio-clippy branch May 25, 2021 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s:accepted This pull request is ready for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants