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

Inheritance aware taint analysis, extended collections support #107

Merged
merged 15 commits into from Oct 9, 2015

Conversation

formanek
Copy link
Contributor

@formanek formanek commented Oct 9, 2015

Another evolution of the taint analysis closes #98, closes #99 and fixes #106.

@h3xstream
Copy link
Member

I didn't had the chance to review fully the commits. But it seems good as the refactoring support all the previous tests and add new one.

High level review:

  • Not sure what this assert is doing. Probably to avoid empty catch ?
  • With the high number of configuration, it would be nice to add tests to check the sanity of the configuration (method summaries).

h3xstream added a commit that referenced this pull request Oct 9, 2015
Inheritance aware taint analysis, extended collections support
@h3xstream h3xstream merged commit c2d4141 into find-sec-bugs:master Oct 9, 2015
@formanek
Copy link
Contributor Author

formanek commented Oct 9, 2015

I am using assert false to indicate that something never happens (if I'm not mistaken). In this case, every implementation of Java platform is required to support UTF-8, so UnsupportedEncodingException should never be thrown. I am thinking of assert commands as inner documentation better than comments, because we are warned if the assertion doesn't hold (when running with -ea parameter), so I'm using these quite often.

Well, adding tests would be always nice, but just extending the configuration was quite time consuming and testing everything would be even worse. It's probably faster just to review them. I will rather implement new detectors and features...

@h3xstream
Copy link
Member

Perfect.
Just throwing an idea for the sanity test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants