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

perf: sanitize build artifacts using ASAN/LSAN #2794

Closed

Conversation

bartlomieju
Copy link
Member

This PR is intended to sanitize more build targets.

Currently only libdeno_test is built and run with ASAN/LSAN

@ry
Copy link
Member

ry commented Aug 19, 2019

Great - so this failed with a very readable error:

Direct leak of 3393160 byte(s) in 1 object(s) allocated from:
    #0 0x5586899ae30d in operator new[](unsigned long) /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:102:3
    #1 0x55868b01e8ee in v8::internal::Snapshot::CreateSnapshotBlob(v8::internal::SnapshotData const*, v8::internal::SnapshotData const*, std::__1::vector<v8::internal::SnapshotData*, std::__1::allocator<v8::internal::SnapshotData*> > const&, bool) (/home/travis/build/denoland/deno/target/debug/deno_core_test+0x28198ee)
    #2 0x558689a262fd in v8::SnapshotCreator::CreateBlob(v8::SnapshotCreator::FunctionCodeHandling) (/home/travis/build/denoland/deno/target/debug/deno_core_test+0x12212fd)
    #3 0x55868cd9a820 in deno_snapshot_new core/libdeno/api.cc:101:37
    #4 0x55868cb88ebd in deno_core_test_bin::isolate::Isolate::snapshot::h8f29014824d4a2a2 core/isolate.rs:471:28
    #5 0x5586899e3af9 in __rust_maybe_catch_panic /rustc/eae3437dfe991621e8afdc82734f4a172d7ddf9b/src/libpanic_unwind/lib.rs:82:7
SUMMARY: AddressSanitizer: 3393160 byte(s) leaked in 1 allocation(s).

It says we're leaking the snapshot blob. That seems not unlikely.

@@ -68,6 +68,8 @@ Deno* deno_new(deno_config config) {
// If no snapshot is provided, we initialize the context with empty
// main source code and source maps.
deno::InitializeContext(isolate, context);
} else {
delete[] d->snapshot_.data;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ry adding this bit makes LSAN happy. I followed this test and it indeed seems to fix the issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But makes tests fail 😓 still digging...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so I figured what causes this leak, however I'm not yet sure how to fix it.

The problem is in will_snapshot test case in //cli/isolate.rs; this test does a snapshot of a short script and then tries to instantiate a new isolate with that snapshot. Isolate.snapshot() returns Snapshot1 which explicitly says in comment:

/// The type returned from deno_snapshot_new. Needs to be dropped.

During creation of new isolate (Isolate::new) this snapshot is passed in deno_config as load_snapshot (which has a type of Snapshot2 not Snapshot1). However there is no way to tell that this is the type of snapshot on C++ side and Rust's borrow checker prevents an explicitly drop after calling libdeno::deno_new.

@bartlomieju
Copy link
Member Author

@ry in light of #2825 I believe Isolate::snapshot will become absolete and this PR won't make much sense. I'm closing it for now and we can revisit this topic after #2825 lands.

@bartlomieju bartlomieju closed this Sep 2, 2019
@bartlomieju
Copy link
Member Author

@ry in light of #2825 I believe Isolate::snapshot will become absolete and this PR won't make much sense. I'm closing it for now and we can revisit this topic after #2825 lands.

After further reviewing latest changes this PR still makes sense

@bartlomieju bartlomieju reopened this Sep 3, 2019
@ry
Copy link
Member

ry commented Sep 3, 2019

@bartlomieju Isolate::snapshot will stick around, it's how deno_typescript makes snapshots.

That said, I think more work needs to be put into fixing the memory ownership situation and merging the Snapshot1 Snapshot2 types.

@ry
Copy link
Member

ry commented Oct 2, 2019

Closing because it's old. We've since removed the ASAN test completely when we moved to "cargo build"... Hoping to bring that back at some point when Rust supports it in stable.

@ry ry closed this Oct 2, 2019
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