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 3 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 new_process.ProcessResult(0, '', False).retcode == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always be true wont it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This reverts to https://github.com/google/fuzzbench/pull/1529/files#diff-f61c598f4891541e693644bbc9016e5841fcddc986ea967eb3e192586a8c57deL134

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe I can also fix it by adding ==0 to return new_process.ProcessResult(0, '', False).

Copy link
Contributor Author

@DonggeLiu DonggeLiu Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the new code (after revert) will always be true.
For example, it will be false if the process fails and returns a non-zero value, right?

Copy link
Contributor

@aeflores aeflores Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The way it was before using a context manager (before a6a5a9a) was:

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

That indeed does not check anything about Popen. new_process.ProcessResult(0, '', False) is just creating a named tuple (see definition here https://github.com/google/fuzzbench/blob/master/common/new_process.py#L67). There is no connection between that line and the call to Popen.

The important fix here though is to stop using the context manager. As per the documentation (https://docs.python.org/3/library/subprocess.html?highlight=popen#subprocess.Popen)

Popen objects are supported as context managers via the with statement: on exit, standard file descriptors are closed, and the process is waited for.

Using a context manager closes file descriptors and waits for the process to terminate, which is probably what is causing problems. This code probably needs to start the process, but not wait for its termination.

My suggestion would be:

subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
return True

If we want to be extra careful, we should check that the subprocess is created successfully, but I think we will get an exception if that were not the case anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @aeflores!
Yeah, your suggestion is cleaner as the old return statement is trivially true.
I've fixed it accordingly : )



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