Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

ethereum libfuzzer integration small change #9547

Merged
merged 8 commits into from
Sep 27, 2018
Merged

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Sep 13, 2018

This PR, contains very few change to avoid maintaining a parity branch for ethereum libfuzzer project (https://github.com/ethereum/ethereum-vm-fuzzer).

Currently parity fuzzing with libfuzzer requires manual changes (or maintaining their own branch which is also difficult and I see it as kind of a blocker for integrating the libfuzzer operation to others ethereum testing projects).

In this pr we expose the pod_account module as public. I do not believe that it is a big issue (pod_state module for instance is already public), I can also reexport 'PodAccount' from pod_state or look for other ways of doing it.

We also add a new test function for EvmTestClient : call_envinfo which allows the fuzzer to fuzz on envinfo. Since it only impact EvmTestClient (test), I think it is a minor change.

@parity-cla-bot
Copy link

It looks like @cheme signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@cheme cheme changed the title ethereum libfuzzer integration small changes ethereum libfuzzer integration small change Sep 13, 2018
@5chdn 5chdn added this to the 2.2 milestone Sep 13, 2018
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M4-core ⛓ Core client code / Rust. labels Sep 13, 2018
@dvdplm
Copy link
Collaborator

dvdplm commented Sep 13, 2018

Currently parity fuzzing with libfuzzer requires manual changes

Can you elaborate on this? Not sure I understand what this entails.

Not sure if it makes sense, but it'd be easier (for me) to review this if I knew what the testing situation (adding new tests? running the current ones?) looks like now and how it'd improve with this change. Thanks!

@cheme
Copy link
Contributor Author

cheme commented Sep 13, 2018

It's an ethereum project (I realize the linked repo is private) that uses https://llvm.org/docs/LibFuzzer.html on both geth and parity (I think aleth also plugged on it at some point but I am not sure). The fuzzer is mainly targetting the evm interpreter.

The manual changes are what is in this PR (not much but enough to be a bad constraint). There is also some rust code in the fuzzer project (which need those small changes and exposes the C fuzz target), but as a first step removing this constraint will be good.

Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

LGTM. I can't think of surprising/incorrect usages of exposing pod_account, and I believe it doesn't break any of our current assumptions.

@@ -194,6 +207,7 @@ impl<'a> EvmTestClient<'a> {
).map_err(EvmTestError::Evm)
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra blank line. Please remove!

@sorpaas sorpaas added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 13, 2018
@5chdn
Copy link
Contributor

5chdn commented Sep 15, 2018

please merge master to update pipelines

@5chdn 5chdn added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 19, 2018
@cheme cheme added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 26, 2018
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm

@cheme cheme merged commit a8f6f5b into openethereum:master Sep 27, 2018
@cheme cheme deleted the evmfuzzer branch September 27, 2018 15:24
dvdplm added a commit that referenced this pull request Sep 27, 2018
…mon-deps

* origin/master:
  ethereum libfuzzer integration small change (#9547)
  cli: remove reference to --no-ui in --unlock flag help (#9616)
  remove master from releasable branches (#9655)
  ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to (#9620)
  RPC: parity_getBlockReceipts (#9527)
  Remove unused dependencies (#9589)
  ignore key_server_cluster randomly failing tests (#9639)
  ethcore: handle vm exception when estimating gas (#9615)
  fix bad-block reporting no reason (#9638)
  Use static call and apparent value transfer for block reward contract code (#9603)
  HF in POA Sokol (2018-09-19) (#9607)
  bump smallvec to 0.6 in ethcore-light, ethstore and whisper (#9588)
  Add constantinople conf to EvmTestClient. (#9570)
@5chdn 5chdn removed the A0-pleasereview 🤓 Pull request needs code review. label Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants