New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 754124 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocked on:
issue 764514



Sign in to add a comment

Null-dereference READ in blink::PersistentBase<blink::LocalFrame,

Project Member Reported by ClusterFuzz, Aug 10 2017

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5791621626200064

Fuzzer: libFuzzer_v8_serialized_script_value_fuzzer
Job Type: mac_libfuzzer_chrome_asan
Platform Id: mac

Crash Type: Null-dereference READ
Crash Address: 0x000000000010
Crash State:
  blink::PersistentBase<blink::LocalFrame,
  blink::DummyPageHolder::GetFrame
  blink::LLVMFuzzerTestOneInput
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=mac_libfuzzer_chrome_asan&range=493100:493156

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5791621626200064

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
 
Cc: msrchandra@chromium.org
Labels: M-62 Test-Predator-Wrong
Owner: reillyg@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL could not provide any possible suspects.
Using Code Search for the file, "DummyPageHolder.cpp" assigning to concern owner.

Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/f791eeeca58c648ee7b55e439e0368129a7db990

@reillyg -- Could you please look into the issue, kindly re-assign if it is not related to your changes.
Thank You.
Cc: reillyg@chromium.org
Owner: jbroman@chromium.org
It looks like g_page_holder is null in this case. That indicates a failure during setup that I cannot explain.

Jeremy, can you take a look? I don't think this is related to my change.
Cc: mmoroz@chromium.org
I suspect the libfuzzer roll is responsible. Perhaps LLVMFuzzerInitialize is no longer being called?

https://chromium.googlesource.com/chromium/src/+/a01f1b580b1dce96e37b737304148d290d6ac006

Comment 4 by mmoroz@chromium.org, Aug 23 2017

Cc: kcc@chromium.org
Hm, that's interesting. And it happens only on Mac. Kostya, could c#3 be a truth? :)

Comment 5 by kcc@chromium.org, Aug 23 2017

There were a few changes done by the Apple folks that may potentially lead to this on Mac, but still unlikely. We have tests that validate that LLVMFuzzerInitialize works as expected. 

Sadly, I have no Mac expertise and can't help debug this. 

Comment 6 by mmoroz@chromium.org, Aug 23 2017

Cc: jbroman@chromium.org infe...@chromium.org
Owner: mmoroz@chromium.org
Thanks for the prompt response! Ok, I'll get a mac and figure out what's going on.

Comment 7 by mmoroz@chromium.org, Aug 31 2017

It looks like jbroman@ is right, but only partially. LLVMFuzzerInitialize is indeed not being called, but it is not being called even with an old libFuzzer revisions. This particular crash doesn't reproduce with an old libFuzzer, because it used not to call LLVMFuzzerTestOneInput with an empty input (size of 0), and now it does call LLVMFuzzerTestOneInput anyway.
On Linux, LLVMFuzzerInitialize is being called properly. Then, LLVMFuzzerTestOneInput even with empty input does not crash.

Status: Started (was: Assigned)
Reproduced with a smaller example:

#include <stddef.h>
#include <stdint.h>

#include <stdio.h>

extern "C" int LLVMFuzzerInitialize(int* argc, char*** argv) {
  printf("Hello from LLVMFuzzerInitialize, argc: %i\n", *argc);
  return *argc;
}

extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
  printf("Hello from LLVMFuzzerTestOneInput, size: %zu\n", size);
  if (size) {
  	return data[0];
  }
  return size;
}


$ out/asan/mac_test_fuzzer /tmp/1 
HELLO: EF->LLVMFuzzerInitialize: 0x0
INFO: Seed: 4290147427
INFO: Loaded 1 modules   (3 guards): 3 [0x10e8384b0, 0x10e8384bc), 
out/asan/mac_test_fuzzer: Running 1 inputs 1 time(s) each.
Running: /tmp/1
Hello from LLVMFuzzerTestOneInput, size: 10
Hello from LLVMFuzzerTestOneInput, size: 10
Executed /tmp/1 in 0 ms
***
*** NOTE: fuzzing was not performed, you have only
***       executed the target code on a fixed set of inputs.
***



If I enable warnings in FuzzerExtFunctions.def, I see the following output:

$ out/asan/mac_test_fuzzer /tmp/1 
WARNING: Failed to find function "LLVMFuzzerInitialize". Reason dlsym(RTLD_DEFAULT, LLVMFuzzerInitialize): symbol not found.
WARNING: Failed to find function "LLVMFuzzerCustomMutator". Reason dlsym(RTLD_DEFAULT, LLVMFuzzerCustomMutator): symbol not found.
WARNING: Failed to find function "LLVMFuzzerCustomCrossOver". Reason dlsym(RTLD_DEFAULT, LLVMFuzzerCustomCrossOver): symbol not found.
<...>

Which means that libFuzzer itself is working fine, i.e. it is looking for the symbols. That leads me to a conclusion that there an issue with how we build stuff on Mac.

$ nm out/asan/obj/testing/libfuzzer/fuzzers/mac_test_fuzzer/mac_test_fuzzer.o
0000000000000000 T _LLVMFuzzerInitialize
0000000000000090 T _LLVMFuzzerTestOneInput
                 U ___asan_init
                 U ___asan_report_load1
                 U ___asan_report_load4
                 U ___asan_version_mismatch_check_v8
                 U ___sanitizer_cov_trace_pc_guard
                 U ___sanitizer_cov_trace_pc_guard_init
0000000000000140 t _asan.module_ctor
                 U _printf
0000000000000120 t _sancov.module_ctor
00000000000001a8 s l___sancov_gen_
00000000000001ac s l___sancov_gen_.2
                 U section$end$__DATA$__sancov_guards
                 U section$start$__DATA$__sancov_guards


_LLVMFuzzerInitialize is there, but it has zero offset (I guess)? Wonder if it could be an issue...
No, that zero offset is not an issue, just verified :)
I've found that 2yo reply from Kostya regarding weak function declarations not working on Mac: http://clang-developers.42468.n3.nabble.com/Build-issues-when-trying-to-use-LibFuzzer-on-Mac-OS-X-tp4045832p4046420.html


Well, there is also a nice-written comment in libFuzzer code (https://chromium.googlesource.com/chromium/llvm-project/compiler-rt/lib/fuzzer.git/+/master/FuzzerExtFunctionsWeak.cpp#9):

// Implementation for Linux. This relies on the linker's support for weak
// symbols. We don't use this approach on Apple platforms because it requires
// clients of LibFuzzer to pass ``-U _<symbol_name>`` to the linker to allow
// weak symbols to be undefined. That is a complication we don't want to expose
// to clients right now.
It looks like I have a fix locally. It consists of two changes:

1) add "-Wl,-U,_LLVMFuzzerCustomCrossOver", to https://cs.chromium.org/chromium/src/testing/libfuzzer/BUILD.gn?l=31

2) enable https://cs.chromium.org/chromium/src/third_party/libFuzzer/src/FuzzerExtFunctionsWeak.cpp?l=16 for LIBFUZZER_APPLE and disable https://cs.chromium.org/chromium/src/third_party/libFuzzer/src/FuzzerExtFunctionsDlsym.cpp?l=15


It works for my small fuzz target, now testing for v8_serialized_script_value_fuzzer.

I'm completely unsure about the second part of my fix. Kostya (kcc@), may I ask you to take a look and comment whether it's feasible to do so?
The fix works for v8_serialized_script_value_fuzzer too. I'm thinking about doing the following:

1) https://1cs.chromium.org/chromium/src/third_party/libFuzzer/src/FuzzerExtFunctionsWeak.cpp?l=16:

add "|| UNDEFINED_WEAK_SYMBOLS_ALLOWED"

2) https://cs.chromium.org/chromium/src/third_party/libFuzzer/src/FuzzerExtFunctionsDlsym.cpp?l=15:

add "&& !UNDEFINED_WEAK_SYMBOLS_ALLOWED"


And then define UNDEFINED_WEAK_SYMBOLS_ALLOWED in Chromium GN config. Thus, we won't break nor complicate things for other libFuzzer customers using Mac, but can make it work for our use case.

Comment 17 by mmoroz@google.com, Sep 6 2017

As per suggestion from Kostya, started a discussion at llvm-dev@ mailing list: http://lists.llvm.org/pipermail/llvm-dev/2017-September/117185.html
Blockedon: 764514
Project Member

Comment 19 by ClusterFuzz, Oct 1 2017

Components: Blink>MemoryAllocator>GarbageCollection
Labels: Test-Predator-AutoComponents
Automatically applying components based on information from OWNERS files. If this seems incorrect, please apply the Test-Predator-Wrong-Components label.
For more information, please see https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md.

The link referenced in the description is no longer valid.

(bulk edit)
Labels: -Test-Predator-AutoComponents Test-Predator-Auto-Components
Project Member

Comment 22 by bugdroid1@chromium.org, Dec 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/004f348aba415381cf5b66801e0e0f26c41baae6

commit 004f348aba415381cf5b66801e0e0f26c41baae6
Author: Max Moroz <mmoroz@chromium.org>
Date: Tue Dec 19 18:18:11 2017

[fuzzer] Add attributes to LLVMFuzzerInitialize definition.

That prevents the linker from dead-stripping the function, as it is not called
directly, it is resolved in the runtime via dlsym().

Bug:  chromium:754124 ,  chromium:787723 
Change-Id: I46a02ef01349f59b7ed944ce1483b7277e234a19
Reviewed-on: https://chromium-review.googlesource.com/833995
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50212}
[modify] https://crrev.com/004f348aba415381cf5b66801e0e0f26c41baae6/test/fuzzer/fuzzer-support.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Dec 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/3ffbef33bcedf4fdc31d02df3290454b2d5f5812

commit 3ffbef33bcedf4fdc31d02df3290454b2d5f5812
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Tue Dec 19 18:57:01 2017

Revert "[fuzzer] Add attributes to LLVMFuzzerInitialize definition."

This reverts commit 004f348aba415381cf5b66801e0e0f26c41baae6.

Reason for revert: Breaks msvc compile: https://build.chromium.org/p/client.v8/builders/V8%20Win64%20-%20msvc/builds/672

Original change's description:
> [fuzzer] Add attributes to LLVMFuzzerInitialize definition.
> 
> That prevents the linker from dead-stripping the function, as it is not called
> directly, it is resolved in the runtime via dlsym().
> 
> Bug:  chromium:754124 ,  chromium:787723 
> Change-Id: I46a02ef01349f59b7ed944ce1483b7277e234a19
> Reviewed-on: https://chromium-review.googlesource.com/833995
> Commit-Queue: Max Moroz <mmoroz@chromium.org>
> Reviewed-by: Andreas Haas <ahaas@chromium.org>
> Reviewed-by: Mathias Bynens <mathias@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#50212}

TBR=ahaas@chromium.org,mmoroz@chromium.org,mathias@chromium.org

Change-Id: Iba35b55ee4d11aca0dfb9cffde7a6a51e0c8e46c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:754124 ,  chromium:787723 
Reviewed-on: https://chromium-review.googlesource.com/834548
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50213}
[modify] https://crrev.com/3ffbef33bcedf4fdc31d02df3290454b2d5f5812/test/fuzzer/fuzzer-support.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Dec 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f9eb31bb8eae3218ce5e17f58dfd920b2648790e

commit f9eb31bb8eae3218ce5e17f58dfd920b2648790e
Author: Max Moroz <mmoroz@chromium.org>
Date: Tue Dec 19 19:44:20 2017

[fuzzer] Declare LLVMFuzzerInitialize with attributes only if V8_OS_MACOSX.

R=ahaas@chromium.org, clemensh@chromium.org, mathias@chromium.org

Bug:  chromium:754124 ,  chromium:787723 
Change-Id: I7eafee50a47ca0ad56a5458f1f232e3ed07c1cca
Reviewed-on: https://chromium-review.googlesource.com/834457
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50217}
[modify] https://crrev.com/f9eb31bb8eae3218ce5e17f58dfd920b2648790e/test/fuzzer/fuzzer-support.cc

Cc: keishi@chromium.org hajimehoshi@chromium.org
 Issue 761328  has been merged into this issue.
Project Member

Comment 26 by bugdroid1@chromium.org, Jan 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e2d52131d743540c0c361845b6979f7d20146b1

commit 7e2d52131d743540c0c361845b6979f7d20146b1
Author: Max Moroz <mmoroz@chromium.org>
Date: Thu Jan 04 22:00:52 2018

Fix v8_serialized_script_value_fuzzer on macOS.

R=jbroman@chromium.org

Bug:  754124 
Change-Id: Ia7dd043982d404afa316be81fb030c9caed9e7c0
Reviewed-on: https://chromium-review.googlesource.com/849476
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527110}
[modify] https://crrev.com/7e2d52131d743540c0c361845b6979f7d20146b1/third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValueFuzzer.cpp

Project Member

Comment 27 by ClusterFuzz, Jan 7 2018

ClusterFuzz has detected this issue as fixed in range 526949:527421.

Detailed report: https://clusterfuzz.com/testcase?key=5791621626200064

Fuzzer: libFuzzer_v8_serialized_script_value_fuzzer
Job Type: mac_libfuzzer_chrome_asan
Platform Id: mac

Crash Type: Null-dereference READ
Crash Address: 0x000000000010
Crash State:
  blink::PersistentBase<blink::LocalFrame,
  blink::DummyPageHolder::GetFrame
  blink::LLVMFuzzerTestOneInput
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=mac_libfuzzer_chrome_asan&range=493100:493156
Fixed: https://clusterfuzz.com/revisions?job=mac_libfuzzer_chrome_asan&range=526949:527421

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5791621626200064

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 28 by ClusterFuzz, Jan 7 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 5791621626200064 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment