Page MenuHomePhabricator

[libFuzzer] communicate through pipe to subprocess for MinimizeCrashInput
ClosedPublic

Authored by ychen on Jan 23 2020, 11:29 PM.

Details

Summary

For CleanseCrashInput, discards stdout output anyway since it is not used.

Also giving TempPath a prefix representing the work it is doing.

These changes are to defend against aggressive PID recycle on windows to reduce the chance of contention on files.

Using pipe instead of file also workaround the problem that when the
process is spawned by llvm-lit, the aborted process keeps a handle to the
output file such that the output file can not be removed. This will
cause random test failures.

https://devblogs.microsoft.com/oldnewthing/20110107-00/?p=11803

Diff Detail

Event Timeline

ychen created this revision.Jan 23 2020, 11:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 23 2020, 11:29 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript

Unit tests: fail. 62152 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 5 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ychen planned changes to this revision.Jan 24 2020, 12:17 AM

Somehow this caused three failures on Linux. I'll take a look.

ychen updated this revision to Diff 240409.Jan 25 2020, 8:04 PM
  • fix two linux test failure

Unit tests: fail. 62195 tests passed, 1 failed and 815 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/lock.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 4 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: pass. 62243 tests passed, 0 failed and 816 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 4 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ychen updated this revision to Diff 240658.Jan 27 2020, 12:44 PM
  • clang-format

Unit tests: pass. 62246 tests passed, 0 failed and 816 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 4 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ychen retitled this revision from [libFuzzer] communicate through pipe to subprocess for CleanseCrashInput MinimizeCrashInput to [libFuzzer] communicate through pipe to subprocess for MinimizeCrashInput.Jan 27 2020, 4:36 PM
ychen edited the summary of this revision. (Show Details)
ychen edited the summary of this revision. (Show Details)
ychen edited the summary of this revision. (Show Details)Jan 29 2020, 2:16 PM

It looks like this patch has at least two independent changes.
Please prefer to send two+ individual changes next time. (even if the patches are tiny).

Vitaly, please review

compiler-rt/lib/fuzzer/FuzzerDriver.cpp
36

any chance to not have any macros?

326

replace 128 with sizeof(TmpBuffer)

ychen updated this revision to Diff 241325.Jan 29 2020, 4:58 PM
  • Address comments
ychen marked 2 inline comments as done.Jan 29 2020, 4:59 PM
ychen added a comment.Jan 29 2020, 5:01 PM
In D73329#1848269, @kcc wrote:

It looks like this patch has at least two independent changes.
Please prefer to send two+ individual changes next time. (even if the patches are tiny).

Thanks. I'll pay closer attention in the future.

Unit tests: pass. 62315 tests passed, 0 failed and 838 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 8 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ychen updated this revision to Diff 241330.Jan 29 2020, 5:43 PM
  • clang-format

Unit tests: pass. 62315 tests passed, 0 failed and 838 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 6 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

rnk removed a reviewer: rnk.Feb 10 2020, 11:32 AM
vitalybuka accepted this revision.Feb 10 2020, 1:25 PM
vitalybuka added inline comments.
compiler-rt/lib/fuzzer/FuzzerDriver.cpp
321

exit code is used to check only success
please make it bool
true - success
false - error

503

This verbosity in temp names is nice but irrelevant to the goal of the patch.
Could you extract this into a separate NFC patch.

This revision is now accepted and ready to land.Feb 10 2020, 1:25 PM
ychen updated this revision to Diff 243698.Feb 10 2020, 4:43 PM
  • make ExecuteCommandWithPopen return bool
  • split out TempPath prefixing part as separate NFC patch
This revision was automatically updated to reflect the committed changes.

Hi, a bisect seems to show that this patch resulted in a series of linker errors when building fuzzers fuchsia:

[48364/49380] LINK user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer
FAILED: user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.map
../../../recipe_cleanup/clang9HU4mg/bin/clang++ -o user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug -fuse-ld=lld -Wl,--threads -Wl,-z,combreloc -Wl,-z,relro -Wl,-z,now -Wl,-z,text -Wl,--pack-dyn-relocs=relr --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -Wl,--color-diagnostics -Wl,-z,max-page-size=4096 -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -Wl,--icf=all -g3 -ffunction-sections -Wl,--gc-sections -fdata-sections -fno-exceptions -fno-rtti -nostartfiles -nolibc -Wl,-dynamic-linker=fuzzer.asan-ubsan/ld.so.1 -fsanitize=fuzzer -fsanitize=undefined -fsanitize=address -L../../zircon/public/gn/config/libc-dummy -Wl,-Map,user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.map -Wl,--start-group @'user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.rsp' -lunwind -Wl,--end-group user.vdso-x64-clang.shlib/obj/system/ulib/zircon/libzircon.so.debug user.fuzzer-x64-asan-ubsan.shlib/obj/system/ulib/c/libc.so.debug user-x64-asan-ubsan.shlib/obj/system/ulib/fdio/libfdio.so.debug && ../../../recipe_cleanup/clang9HU4mg/bin/llvm-objcopy --strip-sections "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug" "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer" && case '-fuse-ld=lld -Wl,--threads -Wl,-z,combreloc -Wl,-z,relro -Wl,-z,now -Wl,-z,text -Wl,--pack-dyn-relocs=relr --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -Wl,--color-diagnostics -Wl,-z,max-page-size=4096 -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -Wl,--icf=all -g3 -ffunction-sections -Wl,--gc-sections -fdata-sections -fno-exceptions -fno-rtti -nostartfiles -nolibc -Wl,-dynamic-linker=fuzzer.asan-ubsan/ld.so.1 -fsanitize=fuzzer -fsanitize=undefined -fsanitize=address -L../../zircon/public/gn/config/libc-dummy' in *build-id=none*) ;; *) ../../prebuilt/tools/buildidtool/linux-x64/buildidtool -build-id-dir ".build-id" -stamp "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.build-id.stamp" -entry "=user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer" -entry ".debug=user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug" ;; esac
ld.lld: error: undefined symbol: fuzzer::OpenProcessPipe(char const*, char const*)
>>> referenced by FuzzerDriver.cpp:321 (compiler-rt/lib/fuzzer/FuzzerDriver.cpp:321)
>>>               fuzzer.o:(fuzzer::ExecuteCommandWithPopen(fuzzer::Command const&, std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> >*)) in archive ../../../recipe_cleanup/clang9HU4mg/lib/clang/11.0.0/lib/x86_64-unknown-fuchsia/libclang_rt.fuzzer.a

... and many more

Could you take a look or revert this patch? Thanks.

I tested and if there will be a revert, 4f3c3bbbf85a1283796e0e80c654779e40ce328e may also need to be reverted since it depends on this patch.

Builder link: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8888652956426122432

ychen added a comment.Feb 12 2020, 2:13 PM

Hi, a bisect seems to show that this patch resulted in a series of linker errors when building fuzzers fuchsia:

 [48364/49380] LINK user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer
 FAILED: user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.map
 ../../../recipe_cleanup/clang9HU4mg/bin/clang++ -o user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug -fuse-ld=lld -Wl,--threads -Wl,-z,combreloc -Wl,-z,relro -Wl,-z,now -Wl,-z,text -Wl,--pack-dyn-relocs=relr --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -Wl,--color-diagnostics -Wl,-z,max-page-size=4096 -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -Wl,--icf=all -g3 -ffunction-sections -Wl,--gc-sections -fdata-sections -fno-exceptions -fno-rtti -nostartfiles -nolibc -Wl,-dynamic-linker=fuzzer.asan-ubsan/ld.so.1 -fsanitize=fuzzer -fsanitize=undefined -fsanitize=address -L../../zircon/public/gn/config/libc-dummy -Wl,-Map,user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.map -Wl,--start-group @'user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.rsp' -lunwind -Wl,--end-group user.vdso-x64-clang.shlib/obj/system/ulib/zircon/libzircon.so.debug user.fuzzer-x64-asan-ubsan.shlib/obj/system/ulib/c/libc.so.debug user-x64-asan-ubsan.shlib/obj/system/ulib/fdio/libfdio.so.debug && ../../../recipe_cleanup/clang9HU4mg/bin/llvm-objcopy --strip-sections "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug" "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer" && case '-fuse-ld=lld -Wl,--threads -Wl,-z,combreloc -Wl,-z,relro -Wl,-z,now -Wl,-z,text -Wl,--pack-dyn-relocs=relr --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -Wl,--color-diagnostics -Wl,-z,max-page-size=4096 -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -Wl,--icf=all -g3 -ffunction-sections -Wl,--gc-sections -fdata-sections -fno-exceptions -fno-rtti -nostartfiles -nolibc -Wl,-dynamic-linker=fuzzer.asan-ubsan/ld.so.1 -fsanitize=fuzzer -fsanitize=undefined -fsanitize=address -L../../zircon/public/gn/config/libc-dummy' in *build-id=none*) ;; *) ../../prebuilt/tools/buildidtool/linux-x64/buildidtool -build-id-dir ".build-id" -stamp "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.build-id.stamp" -entry "=user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer" -entry ".debug=user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug" ;; esac
 ld.lld: error: undefined symbol: fuzzer::OpenProcessPipe(char const*, char const*)
>>> referenced by FuzzerDriver.cpp:321 (compiler-rt/lib/fuzzer/FuzzerDriver.cpp:321)
>>>               fuzzer.o:(fuzzer::ExecuteCommandWithPopen(fuzzer::Command const&, std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> >*)) in archive ../../../recipe_cleanup/clang9HU4mg/lib/clang/11.0.0/lib/x86_64-unknown-fuchsia/libclang_rt.fuzzer.a
 
 ... and many more

Could you take a look or revert this patch? Thanks.

I tested and if there will be a revert, 4f3c3bbbf85a1283796e0e80c654779e40ce328e may also need to be reverted since it depends on this patch.

Builder link: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8888652956426122432

Apologies that I did not add OpenProcessPipe implementation on Fushia. Quick question is there popen/pclose on Fushia? Thanks.

Hi, a bisect seems to show that this patch resulted in a series of linker errors when building fuzzers fuchsia:

 [48364/49380] LINK user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer
 FAILED: user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.map
 ../../../recipe_cleanup/clang9HU4mg/bin/clang++ -o user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug -fuse-ld=lld -Wl,--threads -Wl,-z,combreloc -Wl,-z,relro -Wl,-z,now -Wl,-z,text -Wl,--pack-dyn-relocs=relr --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -Wl,--color-diagnostics -Wl,-z,max-page-size=4096 -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -Wl,--icf=all -g3 -ffunction-sections -Wl,--gc-sections -fdata-sections -fno-exceptions -fno-rtti -nostartfiles -nolibc -Wl,-dynamic-linker=fuzzer.asan-ubsan/ld.so.1 -fsanitize=fuzzer -fsanitize=undefined -fsanitize=address -L../../zircon/public/gn/config/libc-dummy -Wl,-Map,user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.map -Wl,--start-group @'user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.rsp' -lunwind -Wl,--end-group user.vdso-x64-clang.shlib/obj/system/ulib/zircon/libzircon.so.debug user.fuzzer-x64-asan-ubsan.shlib/obj/system/ulib/c/libc.so.debug user-x64-asan-ubsan.shlib/obj/system/ulib/fdio/libfdio.so.debug && ../../../recipe_cleanup/clang9HU4mg/bin/llvm-objcopy --strip-sections "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug" "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer" && case '-fuse-ld=lld -Wl,--threads -Wl,-z,combreloc -Wl,-z,relro -Wl,-z,now -Wl,-z,text -Wl,--pack-dyn-relocs=relr --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -Wl,--color-diagnostics -Wl,-z,max-page-size=4096 -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -Wl,--icf=all -g3 -ffunction-sections -Wl,--gc-sections -fdata-sections -fno-exceptions -fno-rtti -nostartfiles -nolibc -Wl,-dynamic-linker=fuzzer.asan-ubsan/ld.so.1 -fsanitize=fuzzer -fsanitize=undefined -fsanitize=address -L../../zircon/public/gn/config/libc-dummy' in *build-id=none*) ;; *) ../../prebuilt/tools/buildidtool/linux-x64/buildidtool -build-id-dir ".build-id" -stamp "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.build-id.stamp" -entry "=user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer" -entry ".debug=user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug" ;; esac
 ld.lld: error: undefined symbol: fuzzer::OpenProcessPipe(char const*, char const*)
>>> referenced by FuzzerDriver.cpp:321 (compiler-rt/lib/fuzzer/FuzzerDriver.cpp:321)
>>>               fuzzer.o:(fuzzer::ExecuteCommandWithPopen(fuzzer::Command const&, std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> >*)) in archive ../../../recipe_cleanup/clang9HU4mg/lib/clang/11.0.0/lib/x86_64-unknown-fuchsia/libclang_rt.fuzzer.a
 
 ... and many more

Could you take a look or revert this patch? Thanks.

I tested and if there will be a revert, 4f3c3bbbf85a1283796e0e80c654779e40ce328e may also need to be reverted since it depends on this patch.

Builder link: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8888652956426122432

Apologies that I did not add OpenProcessPipe implementation on Fushia. Quick question is there popen/pclose on Fushia? Thanks.

We do not have popen/pclose on Fuchsia. Thanks for getting back quickly!