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.

Sign up for GitHub

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

Revert prev lint fix #1604

Merged
merged 7 commits into from
Jan 13, 2023
Merged

Revert prev lint fix #1604

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5359885
Revert prev lint fix
DonggeLiu Jan 9, 2023
94cc54c
Ignore pylint error
DonggeLiu Jan 9, 2023
e98c11a
Revert all similar lint fixes
DonggeLiu Jan 9, 2023
f002ffb
Remove redundant code
DonggeLiu Jan 11, 2023
2a5f3fa
Merge branch 'master' into fix_local_exp
DonggeLiu Jan 11, 2023
53714cd
Revert "Revert all similar lint fixes"
DonggeLiu Jan 12, 2023
8ab0fb7
Merge branch 'master' into fix_local_exp
DonggeLiu Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions common/gcloud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,9 @@ def run_local_instance(startup_script: Optional[str] = None) -> bool:
"""Does the equivalent of "create_instance" for local experiments, runs
|startup_script| in the background."""
command = ['/bin/bash', startup_script]
with subprocess.Popen(command,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT):
return new_process.ProcessResult(0, '', False)
# pylint: disable=consider-using-with
subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this does anything to check if the command succeeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks the return status of the command in the next line.

Copy link
Contributor

@oliverchang oliverchang Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This seems like the same behaviour as before: https://github.com/google/fuzzbench/pull/1529/files#diff-f61c598f4891541e693644bbc9016e5841fcddc986ea967eb3e192586a8c57deL133

The previous return was a no-op that would've always returned True (side note: is this behaviour actually correct?)

    subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
    return new_process.ProcessResult(0, '', False).retcode == 0

@alan32liu would be good to make this more explicit in the PR description in the future to make it clearer to reviewers :)

return True


def create_instance_template(template_name, docker_image, env, project, zone):
Expand Down
4 changes: 2 additions & 2 deletions fuzzers/eclipser/fuzzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ def eclipser(input_corpus, output_corpus, target_binary):
if os.listdir(input_corpus): # Specify inputs only if any seed exists.
command += ['-i', input_corpus]
print('[eclipser] Run Eclipser with command: ' + ' '.join(command))
with subprocess.Popen(command):
pass
# pylint: disable=consider-using-with
subprocess.Popen(command)


def afl_worker(input_corpus, output_corpus, target_binary):
Expand Down
4 changes: 2 additions & 2 deletions fuzzers/eclipser_aflplusplus/fuzzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ def eclipser(input_corpus, output_corpus, target_binary):
if os.listdir(input_corpus): # Specify inputs only if any seed exists.
command += ['-i', input_corpus]
print('[eclipser] Run Eclipser with command: ' + ' '.join(command))
with subprocess.Popen(command):
pass
# pylint: disable=consider-using-with
subprocess.Popen(command)


def afl_worker(input_corpus, output_corpus, target_binary):
Expand Down
4 changes: 2 additions & 2 deletions fuzzers/fuzzolic_aflplusplus_fuzzy/fuzzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ def fuzzolic(input_corpus, output_corpus, target_binary):
target_binary,
]
print('[fuzzolic] Running Fuzzolic with command: ' + ' '.join(command))
with subprocess.Popen(command):
pass
# pylint: disable=consider-using-with
subprocess.Popen(command)


def afl_worker(input_corpus, output_corpus, target_binary):
Expand Down
4 changes: 2 additions & 2 deletions fuzzers/fuzzolic_aflplusplus_z3/fuzzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ def fuzzolic(input_corpus, output_corpus, target_binary):
target_binary,
]
print('[fuzzolic] Running Fuzzolic with command: ' + ' '.join(command))
with subprocess.Popen(command):
pass
# pylint: disable=consider-using-with
subprocess.Popen(command)


def afl_worker(input_corpus, output_corpus, target_binary):
Expand Down
12 changes: 7 additions & 5 deletions fuzzers/neuzz/fuzzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ def run_neuzz(input_corpus,
'python2', './nn.py', '--output-folder', afl_output_dir, target_binary
]
print('[run_neuzz] Running command: ' + ' '.join(command))
with subprocess.Popen(command, stdout=output_stream, stderr=output_stream):
pass
# pylint: disable=consider-using-with
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this patch changes too much. I think with usage didn't make sense in the first location, but I don't see why you would change these other ones. I think it makes sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I think I should have considered this case by case:

  1. Should use context manager (with) to wait for the process :
    • Here, to enable asan (probably can remove sleep, but I suppose it is better to leave it in this PR?).
  2. Should not use context manager (with):
    • gcloud, to fix the bug in local experiments.
  3. Neutral:
    • All others, using context manager has a neutral effect.

I am inclined to not use with in 3, because it looks cleaner, and it was that way before I attempt to use with to fix linter errors.
@jonathanmetzman what would you think? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should get rid of it in gcloud.py.
I think the other cases should be unchanged. but I'm not sure about them, maybe try a make test-run-... to check them. Either way these files are unimportant so just fix gcloud for now and ignore these please.
Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see!
Done, thanks!

subprocess.Popen(command, stdout=output_stream, stderr=output_stream)
time.sleep(40)
target_rel_path = os.path.relpath(target_binary, os.getcwd())
# Spinning up neuzz
Expand All @@ -101,9 +101,11 @@ def run_neuzz(input_corpus,
target_rel_path, '@@'
]
print('[run_neuzz] Running command: ' + ' '.join(command))
with subprocess.Popen(command, stdout=output_stream,
stderr=output_stream) as neuzz_proc:
neuzz_proc.wait()
# pylint: disable=consider-using-with
neuzz_proc = subprocess.Popen(command,
stdout=output_stream,
stderr=output_stream)
neuzz_proc.wait()


def fuzz(input_corpus, output_corpus, target_binary):
Expand Down