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.

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

[xs] Disable stack-use-after-return detection #8923

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

raphdev
Copy link
Contributor

@raphdev raphdev commented Nov 3, 2022

The detect_stack_use_after_return=1 option in ASAN_OPTIONS set by the runner seemed to cause the fuzzer to not pick up any coverage.

I'm not quite sure why this happens. Any guidance would be appreciated. The fuzzer seems stuck with this flag set:

python infra/helper.py run_fuzzer --corpus-dir ../corpus_xst_json/ xs xst_jsonparse -- seed=1

INFO: seed corpus: files: 1728 min: 1b max: 131336b total: 336638b rss: 92Mb
#64	pulse  cov: 322 ft: 323 corp: 1/1b exec/s: 32 rss: 92Mb
#128	pulse  cov: 322 ft: 323 corp: 1/1b exec/s: 25 rss: 92Mb
#256	pulse  cov: 322 ft: 324 corp: 2/8b exec/s: 23 rss: 93Mb
#512	pulse  cov: 322 ft: 324 corp: 2/8b exec/s: 22 rss: 93Mb
#1024	pulse  cov: 322 ft: 324 corp: 2/8b exec/s: 22 rss: 93Mb

However disabling it seems to make progress, with the same command (note same seed and corpus). After rebuilding image with detect_stack_use_after_return disabled:

python infra/helper.py run_fuzzer --corpus-dir ../corpus_xst_json/ xs xst_jsonparse -- seed=1

INFO: seed corpus: files: 1728 min: 1b max: 131336b total: 336638b rss: 92Mb
#64	pulse  cov: 485 ft: 500 corp: 29/55b exec/s: 32 rss: 93Mb
#128	pulse  cov: 574 ft: 673 corp: 70/239b exec/s: 25 rss: 93Mb
#256	pulse  cov: 749 ft: 1060 corp: 132/707b exec/s: 23 rss: 93Mb
#512	pulse  cov: 834 ft: 1560 corp: 214/1767b exec/s: 22 rss: 93Mb
#1024	pulse  cov: 953 ft: 2344 corp: 353/5213b exec/s: 22 rss: 93Mb

Setting detect_stack_use_after_return=0 fixed it -- local runs picked up coverage with seed corpus, and even a couple of crashes. Until I work around this it would be preferable to gain some coverage, even if we don't detect stack-use-after-return for now.

@google-cla
Copy link

google-cla bot commented Nov 3, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jonathanmetzman
Copy link
Contributor

@kcc @hctim Any idea what could be doing this?

@jonathanmetzman
Copy link
Contributor

The detect_stack_use_after_return=1 option in ASAN_OPTIONS set by the runner seemed to cause the fuzzer to not pick up any coverage.

I'm not quite sure why this happens. Any guidance would be appreciated. The fuzzer seems stuck with this flag set:

python infra/helper.py run_fuzzer --corpus-dir ../corpus_xst_json/ xs xst_jsonparse -- seed=1

INFO: seed corpus: files: 1728 min: 1b max: 131336b total: 336638b rss: 92Mb
#64	pulse  cov: 322 ft: 323 corp: 1/1b exec/s: 32 rss: 92Mb
#128	pulse  cov: 322 ft: 323 corp: 1/1b exec/s: 25 rss: 92Mb
#256	pulse  cov: 322 ft: 324 corp: 2/8b exec/s: 23 rss: 93Mb
#512	pulse  cov: 322 ft: 324 corp: 2/8b exec/s: 22 rss: 93Mb
#1024	pulse  cov: 322 ft: 324 corp: 2/8b exec/s: 22 rss: 93Mb

However disabling it seems to make progress, with the same command (note same seed and corpus). After rebuilding image with detect_stack_use_after_return disabled:

python infra/helper.py run_fuzzer --corpus-dir ../corpus_xst_json/ xs xst_jsonparse -- seed=1

INFO: seed corpus: files: 1728 min: 1b max: 131336b total: 336638b rss: 92Mb
#64	pulse  cov: 485 ft: 500 corp: 29/55b exec/s: 32 rss: 93Mb
#128	pulse  cov: 574 ft: 673 corp: 70/239b exec/s: 25 rss: 93Mb
#256	pulse  cov: 749 ft: 1060 corp: 132/707b exec/s: 23 rss: 93Mb
#512	pulse  cov: 834 ft: 1560 corp: 214/1767b exec/s: 22 rss: 93Mb
#1024	pulse  cov: 953 ft: 2344 corp: 353/5213b exec/s: 22 rss: 93Mb

Setting detect_stack_use_after_return=0 fixed it -- local runs picked up coverage with seed corpus, and even a couple of crashes. Until I work around this it would be preferable to gain some coverage, even if we don't detect stack-use-after-return for now.

How did you figure this out btw?

@raphdev
Copy link
Contributor Author

raphdev commented Nov 10, 2022

I've been working on improving this fuzzer.

After running some local tests on seed corpus, I noticed nothing seemed to have an effect on fuzzer progress -- running for a long time would not show any new functions or paths. Also, producing coverage reports showed areas in code that should be covered by the seed corpus, but were reported uncovered (also the case on oss-fuzz's reports).

When I ran the helper shell command and ran manually it seemed to work fine. At first I thought it might be an issue with linking/portability, since shell runs the project container but other commands run the target on base-runner containers, but we don't really use any 3rd-party libs that need to be statically linked. Eventually I ran with manually ASAN_OPTIONS and it worked, so that told me the project container run worked because it didn't set any. From there, kept eliminating until I found that particular option. Turning it off allowed the fuzzer to pick up coverage.

As an aside, another possibility I explored was that a clash between the default CFLAGS used to build and the ones injected by the base-builder/project container could be causing instrumentation issues. This kind of turned out to be the case but only for coverage builds. Building coverage with ASAN caused some coverage not to be produced if there were errors (i.e., prof files aren't produced if target crashes), which explained why some coverage might not show. We fixed that one upstream and reports look like we'd expect.

The `detect_stack_use_after_return=1` option in `ASAN_OPTIONS` set by the runner seemed to cause the fuzzer to not pick up any coverage. Setting `detect_stack_use_after_return=0` fixed it -- local runs picked up coverage with seed corpus, and even a couple crashes.
@raphdev
Copy link
Contributor Author

raphdev commented Nov 14, 2022

Should we move to an issue to track, and merge the option in the meanwhile? I'm concerned the fuzzer is not picking up any coverage feedback and/or crashes.

@jonathanmetzman jonathanmetzman merged commit 2b5577e into google:master Nov 14, 2022
@jonathanmetzman
Copy link
Contributor

I've been working on improving this fuzzer.

After running some local tests on seed corpus, I noticed nothing seemed to have an effect on fuzzer progress -- running for a long time would not show any new functions or paths. Also, producing coverage reports showed areas in code that should be covered by the seed corpus, but were reported uncovered (also the case on oss-fuzz's reports).

When I ran the helper shell command and ran manually it seemed to work fine. At first I thought it might be an issue with linking/portability, since shell runs the project container but other commands run the target on base-runner containers, but we don't really use any 3rd-party libs that need to be statically linked. Eventually I ran with manually ASAN_OPTIONS and it worked, so that told me the project container run worked because it didn't set any. From there, kept eliminating until I found that particular option. Turning it off allowed the fuzzer to pick up coverage.

As an aside, another possibility I explored was that a clash between the default CFLAGS used to build and the ones injected by the base-builder/project container could be causing instrumentation issues. This kind of turned out to be the case but only for coverage builds. Building coverage with ASAN caused some coverage not to be produced if there were errors (i.e., prof files aren't produced if target crashes), which explained why some coverage might not show. We fixed that one upstream and reports look like we'd expect.

Weird but the output you posted doesn't have a crash and there shouldn't be prof files because this isn't a coverage build.

@raphdev
Copy link
Contributor Author

raphdev commented Nov 17, 2022

I meant the coverage builds separately, source coverage reports did not look like expected. It turned out the source coverage instrumentation and the fuzzer instrumentation weren't working for different reasons. As for the crash, it was fixed since it was a fuzzer issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants