CF points to your CL: 415fd8d8d1060502ad52c095aa18cd05f75d3af5. PTAL.

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

My CL should not have any effect on WebAssembly code (which this seems to be about). I suspect I simply changed heap layout in such a way as to cause a pre-existing bug to surface.

I also doubt this is a security bug, since it hits a CHECK failure.

I'm able to reproduce with the flags "--allow-natives-syntax --gc-interval=458 --expose-gc". What happens is that wasm::GetOwningWasmInstance() returns null when it's not expected to. But from reading the code, I don't see why it shouldn't return null, since the thing it returns is stored in a WeakCell.

Assigning to titzer for further triage, also CC mtrofin who's touched this code a good bit.

I'll take a look.

Here's a more compact repro. 

============
function buildModule(opcode) {
  builder = new WasmModuleBuilder();
  builder.addFunction("main", kSig_i_ii)
    .addBody([
      kExprGetLocal, 0,
      kExprGetLocal, 1,
      kExprI32RemU
    ])
    .exportFunc();
 return builder.instantiate().exports.main;
}
func = buildModule();
func(1, 2);
try {
  delete func['length'];
  // this gc invokes the finalizerB
  gc();
} catch(e) {
  print(e);
}
// this one actually clears the instance
gc();
try {
  eval("func(1, 0)");
} catch(e) {
  print(e);
}

print("done");
===========

to run, reference the wasm mjsunit builder, like this:

d8 --allow-natives-syntax --gc-interval=458 --expose-gc test/mjsunit/wasm/wasm-constants.js test/mjsunit/wasm/wasm-module-builder.cc repro.js

We hold a reference to the wasm instance on each exported function. It seems deleting 'length' on the export actually deletes this reference (verified in debugger).

Digging more (this seems to be about how we use JS side of things, not the area of the code I'm most familiar with)

It looks to me like the bad thing that happens with "delete" is that the object has its properties normalized, and this somehow causes the instance field to be overwritten. And I'm guessing the reason this all goes wrong is that WasmExportedFunction's InstanceType is JS_FUNCTION_TYPE, yet it has a bigger header than JSFunction, leading to incorrect calculations when the Map is initially created. See

https://cs.chromium.org/chromium/src/v8/src/wasm/wasm-js.cc?type=cs&q=CalculateInstanceS&sq=package:chromium&l=881

and

https://cs.chromium.org/chromium/src/v8/src/objects.cc?rcl=c2d2c94fd4960b69e0ea915b9e862f16bea778cb&l=13580

for the incorrect calculations.

+ishell, who might have ideas about how to properly set up the Map for WasmExportedFunction.

I have a fix for the underlying issue, just need to iron out one last wrinkle.

ClusterFuzz has detected this issue as fixed in range 46771:46772.

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

Fuzzer: inferno_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  (owning_instance) != nullptr in runtime-wasm.cc
  v8::internal::GetWasmInstanceOnStackTop
  GetWasmContextOnStackTop
  
Sanitizer: address (ASAN)

Regressed: V8: 46649:46650
Fixed: V8: 46771:46772

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


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.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot