Skip to content

Prevent radamsa in process strategy if mutator plugin is already being used. #1398

Merged
mbarbella-chromium merged 5 commits into
masterfrom
mutator_plugin_conflict
Feb 3, 2020
Merged

Prevent radamsa in process strategy if mutator plugin is already being used. #1398
mbarbella-chromium merged 5 commits into
masterfrom
mutator_plugin_conflict

Conversation

@mpherman2

Copy link
Copy Markdown
Contributor

No description provided.

@googlebot googlebot added the cla: yes CLA signed. label Jan 31, 2020
@mpherman2

Copy link
Copy Markdown
Contributor Author

/gcbrun

Comment thread src/python/bot/fuzzers/libfuzzer.py Outdated
if (strategy_pool.do_strategy(strategy.MUTATOR_PLUGIN_RADAMSA_STRATEGY) and
use_radamsa_mutator_plugin(extra_env)):
use_radamsa_mutator_plugin(extra_env)
) and not fuzzing_strategies.contains(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: keep this close paren before the colon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@mpherman2

Copy link
Copy Markdown
Contributor Author

/gcbrun

1 similar comment
@mbarbella-chromium

Copy link
Copy Markdown
Contributor

/gcbrun

Comment thread src/python/bot/fuzzers/libfuzzer.py Outdated
if (strategy_pool.do_strategy(strategy.MUTATOR_PLUGIN_RADAMSA_STRATEGY) and
use_radamsa_mutator_plugin(extra_env)):
use_radamsa_mutator_plugin(extra_env) and
not fuzzing_strategies.contains(strategy.MUTATOR_PLUGIN_STRATEGY.name)):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the order here is wrong. If line 1706 is true, then line 1707 gets executed and enables the plugin (as it sets the LD_PRELOAD and also logs the message), but then we can end with the false on line 1708, i.e. the strategy will be used but the stats will say it isn't.

Swap these two lines, and maybe add a comment on line 1706 explaining why one strategy needs the other one to be disabled

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a good catch! Thank you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mpherman2

Copy link
Copy Markdown
Contributor Author

/gcbrun

@mpherman2

Copy link
Copy Markdown
Contributor Author

/gcbrun

@mbarbella-chromium mbarbella-chromium merged commit 520327e into master Feb 3, 2020
@mbarbella-chromium mbarbella-chromium deleted the mutator_plugin_conflict branch February 3, 2020 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes CLA signed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants