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

Adds basic inline detector supression function #724

Merged
merged 7 commits into from
Dec 17, 2020

Conversation

josh-richardson
Copy link
Contributor

This is a naive approach to integrating a solhint-ignore-like syntax into slither which allows specific rules to be disabled on the next line.

The syntax is as shown in the two slither-disable-next-line statements in the below contract. Each ignore statement must specify a specific rule being ignored by rule check name.

// SPDX-License-Identifier: MIT
//slither-disable-next-line solc-version
pragma solidity >=0.4.22 <0.8.0;

contract Migrations {
  address public owner = msg.sender;
  uint public last_completed_migration;

  modifier restricted() {
    require(
      msg.sender == owner,
      "This function is restricted to the contract's owner"
    );
    _;
  }
  //slither-disable-next-line external-function
  function setCompleted(uint completed) public restricted {
    last_completed_migration = completed;
  }
}

Running slither against this contract (invoking slither . in a new truffle project having replaced Migrations.sol with the above content) should only yield a single warning, rather than the usual three which are produced without the ignore statements.

I'd include tests, but as far as I can see Slither only has tests for the AST and the detectors.

@CLAassistant
Copy link

CLAassistant commented Dec 10, 2020

CLA assistant check
All committers have signed the CLA.

"""
Check if the result has an ignore comment on the proceeding line, in which case, it is not valid
"""
mapping_elements_with_lines = [
Copy link
Contributor

@disconnect3d disconnect3d Dec 10, 2020

Choose a reason for hiding this comment

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

Since the collection is only iterated upon, we should use a generator expression instead of list comprehension.

Also, while it may be counter-intuitive, it's better to use {..} instead of dict(..) for constructing dicts. The dict is a function call and there is an opcode for the dict literal ({}) which ends up being slightly faster. Though, I wouldn't care about it much. A showcase of this below.

In [1]: def foo(): return dict(a=1,b=2)

In [2]: def bar(): return {'a': 1, 'b': 2}

In [3]: %timeit foo()
188 ns ± 6.21 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [4]: %timeit bar()
143 ns ± 0.64 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [5]: import dis

In [6]: dis.dis(foo)
  1           0 LOAD_GLOBAL              0 (dict)
              2 LOAD_CONST               1 (1)
              4 LOAD_CONST               2 (2)
              6 LOAD_CONST               3 (('a', 'b'))
              8 CALL_FUNCTION_KW         2
             10 RETURN_VALUE

In [7]: dis.dis(bar)
  1           0 LOAD_CONST               1 (1)
              2 LOAD_CONST               2 (2)
              4 LOAD_CONST               3 (('a', 'b'))
              6 BUILD_CONST_KEY_MAP      2
              8 RETURN_VALUE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions implemented.

if match:
ignored_checks = [x.strip() for x in ignore_line_text[match.span()[1] :].split(",")]
matched_ignores = list(filter(lambda c: r["check"] == c, ignored_checks))
if len(matched_ignores) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please confirm if it works properly, but it seems this three lines above could be changed into:

                ignored_checks = (x.strip() for x in ignore_line_text[match.span()[1] :].split(","))
                matched_ignores = filter(lambda c: r["check"] == c, ignored_checks)
                if any(matched_ignores):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if len(code_lines) < ignore_line_index + 1 or ignore_line_index - 1 < 0:
return False
ignore_line_text = code_lines[ignore_line_index - 1]
match = re.match(r"^\s*//\s*slither-disable-next-line\s*", ignore_line_text)
Copy link
Contributor

Choose a reason for hiding this comment

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

image
Leaving it here in case someone wonders what this regex matches (via https://regexper.com/#%5E%5Cs*%5C%2F%5C%2F%5Cs*slither-disable-next-line%5Cs*)

Copy link
Contributor

@disconnect3d disconnect3d Dec 10, 2020

Choose a reason for hiding this comment

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

Since we match from "Start of line" and then allow only for white spaces the suppress detection feature won't work if the comment is made in a line after code. Not sure if it is fine.

Copy link
Contributor Author

@josh-richardson josh-richardson Dec 10, 2020

Choose a reason for hiding this comment

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

If we want to support comments made on a line after code, I don't think regex is the appropriate tool to find them. I think for now this is by design.

We could perhaps use something like this if we want to match comments made after a line of code, however I'd be more comfortable using a parser:

"[^"]+"(*SKIP)(*FAIL)
|
'[^']+'(*SKIP)(*FAIL)
|
(?|
    //(?P<comment>.+)
)

@josh-richardson
Copy link
Contributor Author

Will address these comments this evening.

@josh-richardson
Copy link
Contributor Author

Should all be addressed now, although there's some discussion still to be had about whether we want to support ignore statements placed after a code block.

montyly and others added 3 commits December 17, 2020 12:18
Add --show-ignored-findings flag (both ignore findings from comment and
triaged db)
@montyly montyly merged commit 599ac58 into crytic:dev Dec 17, 2020
@montyly
Copy link
Member

montyly commented Dec 17, 2020

Thanks again @josh-richardson for your effort.

My changes:

  • Added the flag --show-ignored-findings (which will also show the issues triaged with the db)
  • slither-disable-next-line all will disable any detector

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.

4 participants