Revert prev lint fix #1604

DonggeLiu · 2023-01-09T06:50:01Z

Revert a previous lint fix (in Fix lint #1529) to avoid using with on subprocesses, as the context manager made local experiments fail to launch runner instances.
Simplify the previous return statement, which was true by definition.

DonggeLiu · 2023-01-09T06:53:03Z

@dliyanage @kdsjZh could you please test if this works? Thanks!
I am also running a new experiment on my local machine with this change, which seems to work so far.

DonggeLiu · 2023-01-09T06:55:18Z

CI failed due to an expected lint error, as this PR reverts that lint fix.
I will handle that later.

kdsjZh · 2023-01-09T07:14:08Z

Now the runners container started, which seems work from my side! Thanks for the quick fix!

DonggeLiu · 2023-01-09T09:51:37Z

Revert all lint fixes like with subprocess in #1529.
presubmit fails due to the following temporary fuzzers not being in fuzzer list:

eclipser_aflplusplus
neuzz
fuzzolic_aflplusplus_z3
fuzzolic_aflplusplus_fuzzy

DonggeLiu · 2023-01-09T21:39:07Z

@jonathanmetzman A friendly gentle ping on this. Local experiments rely on this (fixes #1594).
I reckon the presubmit error is acceptable, because of the following reason:

Revert all lint fixes like with subprocess in #1529. presubmit fails due to the following temporary fuzzers not being in fuzzer list:

eclipser_aflplusplus

neuzz

fuzzolic_aflplusplus_z3

fuzzolic_aflplusplus_fuzzy

jonathanmetzman

Sorry, but could you explain more?
what is this reverting and how does this fix a problem? THe changed code seems broken IMO

jonathanmetzman · 2023-01-09T21:42:23Z

common/gcloud.py

-        return new_process.ProcessResult(0, '', False)
+    # pylint: disable=consider-using-with
+    subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    return new_process.ProcessResult(0, '', False).retcode == 0


This will always be true wont it?

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

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

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?

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.

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

jonathanmetzman · 2023-01-09T21:42:56Z

common/gcloud.py

-                          stderr=subprocess.STDOUT):
-        return new_process.ProcessResult(0, '', False)
+    # pylint: disable=consider-using-with
+    subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)


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

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

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 :)

jonathanmetzman · 2023-01-11T14:40:42Z

fuzzers/neuzz/fuzzer.py

@@ -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


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.

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

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?).

Should not use context manager (with):

gcloud, to fix the bug in local experiments.

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!

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!

I see!
Done, thanks!

This reverts commit e98c11a.

oliverchang · 2023-01-13T00:32:09Z

common/gcloud.py

-                          stderr=subprocess.STDOUT):
-        return new_process.ProcessResult(0, '', False)
+    # pylint: disable=consider-using-with
+    subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)


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 :)

Navidem · 2023-01-13T02:17:45Z

With this PR, I do not get No snapshot data warning, but am getting this:
WARNING:root:Corpus not found for cycle: 3. Extras: {'fuzzer': 'centipede', 'benchmark': 'libpng-1.6.38', 'trial_id': '35', 'cycle': '3'}

Navidem

LGTM

Navidem · 2023-01-13T02:26:31Z

I think we can merge this.

Revert prev lint fix

5359885

DonggeLiu requested a review from jonathanmetzman January 9, 2023 08:23

DonggeLiu added 2 commits January 9, 2023 19:54

Ignore pylint error

94cc54c

Revert all similar lint fixes

e98c11a

jonathanmetzman requested changes Jan 9, 2023

View reviewed changes

DonggeLiu added 2 commits January 11, 2023 13:30

Remove redundant code

f002ffb

Merge branch 'master' into fix_local_exp

2a5f3fa

DonggeLiu requested a review from jonathanmetzman January 11, 2023 02:37

jonathanmetzman requested changes Jan 11, 2023

View reviewed changes

DonggeLiu added 2 commits January 13, 2023 09:21

Revert "Revert all similar lint fixes"

53714cd

This reverts commit e98c11a.

Merge branch 'master' into fix_local_exp

8ab0fb7

DonggeLiu requested review from jonathanmetzman and Navidem January 12, 2023 22:33

oliverchang approved these changes Jan 13, 2023

View reviewed changes

Navidem approved these changes Jan 13, 2023

View reviewed changes

DonggeLiu merged commit 2749078 into master Jan 13, 2023

DonggeLiu deleted the fix_local_exp branch January 13, 2023 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert prev lint fix #1604

Revert prev lint fix #1604

DonggeLiu commented Jan 9, 2023 •

edited

Loading

DonggeLiu commented Jan 9, 2023

DonggeLiu commented Jan 9, 2023

kdsjZh commented Jan 9, 2023

DonggeLiu commented Jan 9, 2023

DonggeLiu commented Jan 9, 2023

jonathanmetzman left a comment

jonathanmetzman Jan 9, 2023

DonggeLiu Jan 9, 2023

DonggeLiu Jan 9, 2023

DonggeLiu Jan 9, 2023 •

edited

Loading

aeflores Jan 10, 2023 •

edited

Loading

DonggeLiu Jan 11, 2023

jonathanmetzman Jan 9, 2023

DonggeLiu Jan 9, 2023

oliverchang Jan 13, 2023 •

edited

Loading

jonathanmetzman Jan 11, 2023

DonggeLiu Jan 11, 2023

jonathanmetzman Jan 12, 2023

DonggeLiu Jan 12, 2023

oliverchang Jan 13, 2023 •

edited

Loading

Navidem commented Jan 13, 2023

Navidem left a comment

Navidem commented Jan 13, 2023

Revert prev lint fix #1604

Revert prev lint fix #1604

Conversation

DonggeLiu commented Jan 9, 2023 • edited Loading

DonggeLiu commented Jan 9, 2023

DonggeLiu commented Jan 9, 2023

kdsjZh commented Jan 9, 2023

DonggeLiu commented Jan 9, 2023

DonggeLiu commented Jan 9, 2023

jonathanmetzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonggeLiu Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

aeflores Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverchang Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverchang Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Navidem commented Jan 13, 2023

Navidem left a comment

Choose a reason for hiding this comment

Navidem commented Jan 13, 2023

DonggeLiu commented Jan 9, 2023 •

edited

Loading

DonggeLiu Jan 9, 2023 •

edited

Loading

aeflores Jan 10, 2023 •

edited

Loading

oliverchang Jan 13, 2023 •

edited

Loading

oliverchang Jan 13, 2023 •

edited

Loading