Regression range points to 0aef84da31d68d792e50d3e9ecc8458c63b2eac6.

 Issue 740369  has been merged into this issue.

I suspect this is a false positive.

The first reported leak should be deallocated by List<T>'s destructor. I applied the attached patch, and appended to the LSAN_OPTIONS report_objects=1.

This first alleged leak was then found reported freed (by the printf in the patch) further below.

I'll try and find an owner on the lsan side to help me dig into that side.

Deleted: 0001-print.patch 626 bytes

0001-print.patch 626 bytes Download

Not sure how to interpret printf.patch.
Could you try add :

  PerThreadAssertData() {
    printf("PerThreadAssertData: %p\n", this);
...
  }

  ~PerThreadAssertData() {
    printf("~PerThreadAssertData: %p\n", this);
...
  }

 Issue 740495  has been merged into this issue.

Apologies, the printf was useful for a related case (but release instead of debug) - I managed to criss-cross the issues.

On this one: I removed the printf and replaced with the ctor/dtor reports as suggested in #5. Please see attached for an output.

I believe the report claims 0x605000076510 is the leaked object, however, there is a pair ctor/dtor on that pointer above.

Deleted: report.txt 302 KB

report.txt 302 KB View Download

reassigning - based on comment 7.

I reproduced the bug, but I have no clue what is going on yet.
Seems like object is correctly destroyer, but lsan still thinks it's a leak.
Maybe it's a data-race in allocator.

BTW. The only relevant part of 0aef84da31d68d792e50d3e9ecc8458c63b2eac6 is:
Local<ObjectTemplate> test_template = ObjectTemplate::New(isolate);
  global_template->Set(
      String::NewFromUtf8(isolate, "testRunner", NewStringType::kNormal)
          .ToLocalChecked(),
      test_template);

if I sync to 0aef84da31d68d792e50d3e9ecc8458c63b2eac6~1 and add this lines, leaks come back, if I remove these lines on master, it disappears.

Mircea, Is there any idea if this is relevant?

That's strange. At a first glance, I'm not sure why those lines may be relevant - the code there is similar to stuff we've been doing around that place. Isolates are supposed to be entered by one thread at a time, and the way that function is called should ensure that.

Now, the only difference I'm seeing from the other cases there is that, for testRunner, I first attach it to the global object and then attach its functions; the other cases (e.g. realm_template) do it vice-versa; however - your repro does no attaching of functions anyway.

Your suspicion that this may be a data race, would that be on the lsan side, or on the v8 side?

lsan side

lsan needs to suspend threads before leak detection.
For some reason it not even try to suspend the thread which created these objects.

Looking...

Your patch enabled this fork() when multiple threads where already created.
https://cs.chromium.org/chromium/src/v8/src/d8-posix.cc?q=src/d8-posix.cc:513:15&sq=package:chromium&l=513

Asan does not support fork() with threads.

Btw. Sometimes, I see dead locks. With is probably it's also result of fork with threads.

I'm confused - that fork() is in pre-existing code, and not in code I added with 0aef84da31d68d792e50d3e9ecc8458c63b2eac6. 

Could you elaborate?

Thanks!

I don't know details but I can see this if I set CHECK(0) before the fork disable/enable: 
global_template->Set(
      String::NewFromUtf8(isolate, "testRunner", NewStringType::kNormal)
          .ToLocalChecked(),
      test_template);

Interesting (actually, more like surprising)! Thanks, I'll take a look.

Here's why we call 'system', which is where that fork() call happens: the fuzzer-synthesized test calls at a point a random function on a random property on the global object. The way we get such a property/function is by first listing in an array all properties of that object, then calculating a random position in that array.

To cut to the chase, when we add the testRunner property to the global object (which is what my CL does), it so happens it sometimes makes "os" and then "system" be the chosen property and then function.

From what I see, the implementation of system is fine - we want to fork there, because the point of the API is to start a process.


From what Vitaly is saying, I think it's safe to assume we shouldn't ever call 'system' under asan fuzzers.

I suppose we could pass a flag to d8 that means "running under fuzzer", and then just make 'system' a no-op?

Any other ideas?

Hi Mircea! Clusterfuzz sherifing is sometimes about picking likely owners or even (if you are feeling humorous) "victims" from the V8 team to debug an issue. Bisection pointed to your CL. The answer may very well be to pass on to the appropriate owner, but the clusterfuzz sheriff is not the "owner" of the bug. The V8 team overall is, and the sheriff hands the issues that come in out.

How about in this case, maybe you can continue the bisection given what you know thus far on the bug...thanks for the help! :)

Oh - I added Michael hoping he may have some background in the fuzzer; not as a sherriff. I can follow-up offline with more of the v8 team to figure out what we do here. I think we understand the source of the problem, it's not something more bisecting would help, because it's an unfortunate confluence of events (the random thing the fuzzer invokes happens to be the one unsupported by asan)

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

commit ae5de6184ee23e91cfe64550b28165884e4387cc
Author: Mircea Trofin <mtrofin@chromium.org>
Date: Tue Jul 25 14:42:44 2017

[d8] enable os.system only when requested

os.system uses fork(), which is not supported by ASAN/LSAN. Some fuzz tests
consist of js code that randomly picks properties and functions and calls them.
Sometimes, this combination means ASAN will report false positives.

Bug:  chromium:740361 
Change-Id: Id8d517263251a1fe88abadd33b0225c664b00498
Reviewed-on: https://chromium-review.googlesource.com/580313
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Mircea Trofin <mtrofin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46876}
[modify] https://crrev.com/ae5de6184ee23e91cfe64550b28165884e4387cc/src/d8-posix.cc
[modify] https://crrev.com/ae5de6184ee23e91cfe64550b28165884e4387cc/src/d8.cc
[modify] https://crrev.com/ae5de6184ee23e91cfe64550b28165884e4387cc/src/d8.h
[modify] https://crrev.com/ae5de6184ee23e91cfe64550b28165884e4387cc/tools/linux-tick-processor

ClusterFuzz has detected this issue as fixed in range 46875:46876.

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

Fuzzer: inferno_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: Direct-leak
Crash Address: 
Crash State:
  v8::internal::PerThreadAssertScope<
  PerThreadAssertScopeDebugOnly
  v8::internal::Heap::CollectGarbage
  
Sanitizer: address (ASAN)

Regressed: V8: 45239:45241
Fixed: V8: 46875:46876

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


See https://github.com/google/clusterfuzz-tools for more information.

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

 Issue 742631  has been merged into this issue.

ClusterFuzz testcase 5023940329340928 is verified as fixed, so closing issue as verified.

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