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

Fix shadowing-function detector and enhance inheritance-graph #166

Merged
merged 18 commits into from Feb 7, 2019

Conversation

Xenomega
Copy link
Member

@Xenomega Xenomega commented Feb 6, 2019

This pull request aims to resolve #165 . It fixes the shadowing-function detector and extends the inheritance-graph printer.

Changes:
-Fixed an issue where the shadowing-function detector would return all inherited functions (even if not shadowed).
-Added an internal directory under detectors to house detectors which are not used for vulnerabilities, but as helpers throughout the program (such as for the inheritance-graph printer).
-Renamed the argument for shadowing-function detector to shadowing-function-internal
-Updated inheritance-graph printer to highlight all function collisions (shadowing + shadowed functions, not just shadowing).
-Updated inheritance-graph printer to consider modifier collisions (not just functions).
-Updated inheritance-graph printer to highlight collisions via C3 linearization (and added a relevant internal detector to help with detection of all c3 linearization function collisions).

Note: Unimplemented functions and their later implementations will not count as collisions currently (and will not be highlighted). This can easily be changed if desired.

…highlight collisions via C3 linearization.

* Updated shadowing-function-internal detector to ignore unimplemented functions.
* Updated inheritance-graph printer to highlight collisions (shadowed + shadowing) instead of just shadowing.
@Xenomega Xenomega requested a review from montyly February 6, 2019 02:19
@montyly
Copy link
Member

montyly commented Feb 6, 2019

Some notes:

  • After some tests, I am not sure we should highlight the shadowing functions. It makes the use of the printer a bit ambigious (ex: you might think that a function is shadowed, while its the shadowing function). We could use another color, but it's not necessary good to have too many code colors. Another solution is to write the canonical name of the shadowing function next to the shadowed function's name, any thoughts?

  • We could do the same for the state variables. Right now, the derived contracts show all the state variables, while it could only show the new ones (and in red the shadowed ones, as it's probably a bug)

  • My first idea of having detectors that are not 'calleable' from the cli command was a bad idea. We can probably just create a slither.utils.inheritance module with functions that provide the same info

@Xenomega Xenomega changed the title Fix shadowing-function detector and enhance inheritance-graph [WIP] Fix shadowing-function detector and enhance inheritance-graph Feb 6, 2019
@Xenomega
Copy link
Member Author

Xenomega commented Feb 6, 2019

For simplicity in distinguishing commits, the following changes have been since @montyly 's above post:

  • Internal detectors were removed and logic was moved to slither.utils.inheritance_analysis. This provides functions to obtain all shadowing/shadowed function pairs.
    • detect_direct_function_shadowing(contract) will obtain all shadowing via direct line of inheritance for a given contract.
    • detect_c3_function_shadowing(contract) will obtain all indirect shadowing through C3 linearization.
    • detect_function_shadowing(contracts, direct_shadowing=True, indirect_shadowing=True) combines the results from each into pairs which more simplistically state what is shadowed, and what is shadowing. This is used to obtain clean results for the inheritance-graph printer.
  • inheritance-graph was modified to only highlight shadowed functions.

TODO:

  • Currently detect_function_shadowing returns overshadowed pairs for c3 linearization that may contain duplicates, while the same definition cannot actually appear more than once.
  • Add state variable shadowing helpers to inheritance_analysis, plug it in to inheritance-graph.
  • Determine how to connect shadowing vs shadowed functions (such that it is easy for users to read: coloring can become difficult to understand and not provide verbose-enough output for the user to distinguish what collisions are ocurring).

… include c3 collisions which end up neutralized by later inheriting contracts in a multi-inheritance scheme.
* Added relevant test for inheritance_graph.
* Temporarily tweaked again to highlight shadowing functions instead of shadowed.
… in the parent contract, instead of inherited.

* Added highlighting for state variables which overshadow others through inheritance.
* Fixed a bug where special formatting for contract-type state variables wouldn't ever be used.
@Xenomega
Copy link
Member Author

Xenomega commented Feb 6, 2019

For documentation:

To this point, all TODO's are complete (save for the last). Additionally:
-If a contract has multiple inheritance, edges will be numbered in the order of declaration.
-Fixed a bug where special output formatting for state variables of type "Contract" would never be reached.

…terpretted as "%0" and will be used as the title/tooltip for all blank space.
* Changed detect_function_shadowing to return all c3 shadowing results, not just top-level (as super calls might make these results relevant).
* Changed detect_function_shadowing to return the contract_scope as well (the contract where the issue is first detected, needed for indirect shadowing information).
* Updates relevant test to test inheritance-graph with complex direct/indirect shadowing schemes.
…nly provide additional information for indirect conflicts.

* Moved indirect conflicts into the affected contract node, instead of splitting information among all affected.
* Fixed a bug where c3 shadowing detection could return the same function twice in a collision chain (not actually possible).
@Xenomega
Copy link
Member Author

Xenomega commented Feb 7, 2019

This PR is ready for review now. The final behavior for inheritance-graph is as follows:

  • Edges are numbered according to order of inheritance.
  • Variable shadowing will be shown in red.
  • Direct function shadowing will be shown in orange.
  • Additional information about indirect shadowing via c3 linearization will be shown in an additional text box at the bottom of any affected node.

Example:
inheritance-graph

Other supporting changes/bugfixes are detailed throughout this thread.

@Xenomega Xenomega changed the title [WIP] Fix shadowing-function detector and enhance inheritance-graph Fix shadowing-function detector and enhance inheritance-graph Feb 7, 2019
@montyly montyly merged commit e1d8d50 into dev Feb 7, 2019
@montyly montyly deleted the dev-inheritance branch February 8, 2019 12:59
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.

None yet

2 participants