Ensure rss_limit_mb is set to 0 for all Chromium/Google tasks - #5466
tbantikyan wants to merge 2 commits into
Conversation
Bug: crbug.com/555763447 In order to ensure that no memory limit is present for our fuzzing engines, the `rss_limit_mb` flag must be set to 0. This was not being done for corpus pruning tasks and for testcases that were manually uploaded.
49f4eef to
be55e5d
Compare
g-ortuno
left a comment
g-ortuno
left a comment
There was a problem hiding this comment.
Phew that was hard. I didn't get to look at the libfuzzer tests yet!
| workdir = engine_common.create_temp_fuzzing_dir('workdir') | ||
| timeout = max_time + _CLEAN_EXIT_SECS | ||
| args = [ | ||
| minimize_arguments = self._get_arguments(target_path) |
There was a problem hiding this comment.
IIUC, this gets the current arguments for the fuzzer. Does that mean this would return timeout_per_input? If so, we probably want to override it like we do in the reproduce task.
There was a problem hiding this comment.
Yes, it does return timeout_per_input. I'm thinking it makes more sense to delete the argument than to do what reproduce is doing. In deleting, we are preserving the functionality that existed before this CL, as the timeout_per_input value is not set in this method. Also, I think that changing environment variables is a little like working with global variables and makes the code harder to reason about, so I'd prefer to avoid it.
| """ | ||
| runner = libfuzzer.get_runner(target_path) | ||
| libfuzzer.set_sanitizer_options(target_path) | ||
| arguments = fuzzer_options.FuzzerArguments.from_list(arguments) |
There was a problem hiding this comment.
Apparently, from_list can return None if a regex fails, can we assert arguments is not None?
There was a problem hiding this comment.
Good catch. I moved this logic into _add_rss_limit_if_missing.
I made it return arguments with no changes if from_list returns None, as I don't want to introduce a failures to tasks that previously were not failing. I'm not sure how common it is that we'd get an invalid regex; if it's uncommon enough, I'm thinking it's fine to forgo setting rss_limit_mb.
| # get_fuzz_timeout returns a negative value. | ||
| return -fuzz_timeout | ||
|
|
||
| def _add_rss_limit_if_missing(self, target_path, arguments): |
There was a problem hiding this comment.
The changes in this file are defensive, right? We use --rss_limit_mb=0 as the default value, which is stored in the DB; when we retrieve the arguments and pass them to the functions in these file as arguments, they will use the default value already.
There was a problem hiding this comment.
In many cases, what you said is correct. The changes are important for uploaded testcases, though, as those will not already have --rss_limit_mb=0.
- Handle case where `fuzzer_options.FuzzerArguments.from_list` returns None - Factor `rss_limit_mb` logic out from `get_arguments` method - Improve centipede/engine.py documentation with FIXME and clarity comment
| # get_fuzz_timeout returns a negative value. | ||
| return -fuzz_timeout | ||
|
|
||
| def _add_rss_limit_if_missing(self, target_path, arguments): |
There was a problem hiding this comment.
In many cases, what you said is correct. The changes are important for uploaded testcases, though, as those will not already have --rss_limit_mb=0.
| """ | ||
| runner = libfuzzer.get_runner(target_path) | ||
| libfuzzer.set_sanitizer_options(target_path) | ||
| arguments = fuzzer_options.FuzzerArguments.from_list(arguments) |
There was a problem hiding this comment.
Good catch. I moved this logic into _add_rss_limit_if_missing.
I made it return arguments with no changes if from_list returns None, as I don't want to introduce a failures to tasks that previously were not failing. I'm not sure how common it is that we'd get an invalid regex; if it's uncommon enough, I'm thinking it's fine to forgo setting rss_limit_mb.
| workdir = engine_common.create_temp_fuzzing_dir('workdir') | ||
| timeout = max_time + _CLEAN_EXIT_SECS | ||
| args = [ | ||
| minimize_arguments = self._get_arguments(target_path) |
There was a problem hiding this comment.
Yes, it does return timeout_per_input. I'm thinking it makes more sense to delete the argument than to do what reproduce is doing. In deleting, we are preserving the functionality that existed before this CL, as the timeout_per_input value is not set in this method. Also, I think that changing environment variables is a little like working with global variables and makes the code harder to reason about, so I'd prefer to avoid it.
2 participants
Bug: crbug.com/555763447
In order to ensure that no memory limit is present for our fuzzing engines, the
rss_limit_mbflag must be set to 0. This was not being done for corpus pruning tasks and for testcases that were manually uploaded.