[wasm][debugger] - RuntimeError: memory access out of bounds #51859

pavelsavara · 2021-04-26T12:11:23Z

Fixes #51276

fix null reference in describe_value() + add test for it
fix null reference in PDB load
improved handling of targetCrashed in Inspector

On top of that

improve Makefile to allow passing test arguments together with filters
add windows chrome to probe path
include debugging payload into test failure exception for easier debugging
make it possible to compile with WasmBuildNative=true
make it possible to compile with Debug runtime
enable Mono logging in debug mode

ghost · 2021-04-26T12:11:30Z

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Issue Details

Addressing #51276

fix null reference in describe_value()
minimal test case

On top of that

improve Makefile to allow passing test arguments together with filters
add windows chrome to probe path
include debugging payload into test failure exception for easier debugging
align build configuration of the runtime with the test

Author:	pavelsavara
Assignees:	-
Labels:	`area-Debugger-mono`
Milestone:	-

ghost · 2021-04-26T16:12:21Z

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Addressing #51276

fix null reference in describe_value()
minimal test case

On top of that

improve Makefile to allow passing test arguments together with filters
add windows chrome to probe path
include debugging payload into test failure exception for easier debugging
align build configuration of the runtime with the test

Author:	pavelsavara
Assignees:	-
Labels:	`arch-wasm`, `area-Debugger-mono`
Milestone:	-

pavelsavara · 2021-04-26T16:18:09Z

There are few more places which throw memory access out of bounds, converted to draft.

pavelsavara · 2021-05-02T07:54:03Z

/azp list

azure-pipelines · 2021-05-02T07:54:08Z

Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

azure-pipelines · 2021-05-02T07:54:48Z

Commenter does not have sufficient privileges for PR 51859 in repo dotnet/runtime

pavelsavara · 2021-05-02T07:55:11Z

/azp list

azure-pipelines · 2021-05-02T07:55:16Z

Commenter does not have sufficient privileges for PR 51859 in repo dotnet/runtime

pavelsavara · 2021-05-02T07:59:34Z

/azp run runtime

azure-pipelines · 2021-05-02T07:59:38Z

Commenter does not have sufficient privileges for PR 51859 in repo dotnet/runtime

CoffeeFlux · 2021-05-02T13:32:23Z

/azp run runtime

azure-pipelines · 2021-05-02T13:32:47Z

Azure Pipelines successfully started running 1 pipeline(s).

src/mono/wasm/debugger/DebuggerTestSuite/Inspector.cs

src/mono/wasm/debugger/DebuggerTestSuite/SteppingTests.cs

src/mono/wasm/debugger/tests/debugger-test/debugger-test.csproj

src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs

src/mono/mono/mini/mini-wasm-debugger.c

src/mono/wasm/debugger/DebuggerTestSuite/Inspector.cs

src/mono/wasm/debugger/DebuggerTestSuite/SteppingTests.cs

radical · 2021-05-04T17:16:18Z

IIUC, the real issue here is when you are paused "before the method has started" .. and try to look at a local, then that is NULL.

I would suggest:

Test something like:

             await SetBreakpointInMethod("debugger-test.dll", "WithStringBuilder", "OuterMethod", 0);
             await EvaluateAndCheck("window.setTimeout(function() { invoke_static_method('[debugger-test] WithStringBuilder:OuterMethod'); }, 1);", "dotnet://debugger-test.dll/debugger-test.cs", 561, 5, "OuterMethod");

             await StepAndCheck(StepKind.Into, "dotnet://debugger-test.dll/debugger-test.cs", 566, 5, "InnerMethod", times: 2,
                 locals_fn: async (locals) => await CheckProps(locals, new
                 {
                     sb = TObject("System.Text.StringBuilder", is_null: true)
                 }, "locals"));

             await StepAndCheck(StepKind.Into, "dotnet://debugger-test.dll/debugger-test.cs", 567, 9, "InnerMethod", times: 1,
                 locals_fn: async (locals) => await CheckProps(locals, new
                 {
                     sb = TObject("System.Text.StringBuilder")
                 }, "locals"));

this breaks at OuterMethod
steps into InnerMethod, and pauses at the opening brace
We check for locals there, which should be null (this should cause the crash without your fix)
Then we step once again, so sb gets initialized
And we can check that it's not null now
I would also suggest using a simple type, like one of the classes in the debugger tests, instead of StringBuilder
And add similar tests for:
- string, primitive types
- valuetypes
- generic types

- fix null reference in PDB load - improved handling of targetCrashed in Inspector - improve Makefile to allow passing test arguments together with filters - add windows chrome to probe path - include debugging payload into test failure exception for easier debugging - make it possible to compile with WasmBuildNative=true - make it possible to compile with Debug runtime - enable Mono logging in debug mode

lewing

Lots of nice improvements here. I'm doing a local testing before approving because of the CI situation.

radical · 2021-05-05T17:32:09Z

For the cases that are actually being fixed here, IMHO, the new tests should be explicit that they are testing the case of looking at locals before they get initialized. So, maybe name it like that, and the test code can be like:

1        public static int OuterMethod()
2        {
3			 MONO_TYPE_CLASS mtc;
4			 Console.WriteLine("break here"); // breakpoint here             
5            mtc = new MONO_TYPE_CLASS();
6            Console.WriteLine($"mtc: {mtc}");
7        }

Set breakpoint on line 4. Get the locals there:

var pause_location = await EvaluateAndCheck(...
var locals = await GetProperties(pause_location["callFrames"][0]["callFrameId"].Value<string>());

Otherwise, the tests seem like they are accidentally checking the case.

radical · 2021-05-05T17:35:27Z

With a clean DEBUG runtime build, the DebuggerTests.AssignmentTests.InspectDecimalAssignmentsDuringSteppingIn fails on macOS with:

info: DebuggerTests.Inspector[0] console.trace: * Assertion at /Users/radical/dev/r2/src/mono/mono/mini/debugger-engine.c:765, condition `req->refcount' not met
info: DebuggerTests.Inspector[0] console.error: * Assertion at /Users/radical/dev/r2/src/mono/mono/mini/debugger-engine.c:765, condition `req->refcount' not met

{
  "timestamp": 1620228774790.821,
  "exceptionDetails": {
    "exceptionId": 3,
    "text": "Uncaught",
    "lineNumber": 37,
    "columnNumber": 2,
    "scriptId": "8",
    "url": "http://localhost:9400/dotnet.js",
    "stackTrace": {
      "callFrames": [
        {
          "functionName": "quit_",
          "scriptId": "8",
          "url": "http://localhost:9400/dotnet.js",
          "lineNumber": 37,
          "columnNumber": 2
        },
        {
          "functionName": "exit",
          "scriptId": "8",
          "url": "http://localhost:9400/dotnet.js",
          "lineNumber": 11616,
          "columnNumber": 2
        },
        {
          "functionName": "_exit",
          "scriptId": "8",
          "url": "http://localhost:9400/dotnet.js",
          "lineNumber": 6056,
          "columnNumber": 6
        },
        {
          "functionName": "monoeg_assertion_message",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 1889128
        },
        {
          "functionName": "mono_assertion_message",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 1889214
        },
        {
          "functionName": "mono_de_ss_req_release",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 1830351
        },
        {
          "functionName": "mono_de_process_single_step",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 1831800
        },
        {
          "functionName": "mono_wasm_single_step_hit",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 1810692
        },
        {
          "functionName": "do_debugger_tramp",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 171020
        },
        {
          "functionName": "interp_exec_method",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 145696
        },
        {
          "functionName": "interp_runtime_invoke",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 36309
        },
        {
          "functionName": "mono_jit_runtime_invoke",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 1562745
        },
        {
          "functionName": "do_runtime_invoke",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 902006
        },
        {
          "functionName": "mono_runtime_try_invoke",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 904955
        },
        {
          "functionName": "mono_runtime_invoke",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 921755
        },
        {
          "functionName": "mono_wasm_invoke_method",
          "scriptId": "11",
          "url": "http://localhost:9400/dotnet.wasm",
          "lineNumber": 0,
          "columnNumber": 2954696
        },
        {
          "functionName": "",
          "scriptId": "8",
          "url": "http://localhost:9400/dotnet.js",
          "lineNumber": 1430,
          "columnNumber": 21
        },
        {
          "functionName": "ccall",
          "scriptId": "8",
          "url": "http://localhost:9400/dotnet.js",
          "lineNumber": 644,
          "columnNumber": 17
        },
        {
          "functionName": "",
          "scriptId": "8",
          "url": "http://localhost:9400/dotnet.js",
          "lineNumber": 656,
          "columnNumber": 11
        },
        {
          "functionName": "managed__debugger_test__DebuggerTests_MONO_TYPE_VALUETYPE2_OuterMethod",
          "scriptId": "48",
          "url": "https://mono-wasm.invalid/managed__debugger_test__DebuggerTests_MONO_TYPE_VALUETYPE2_OuterMethod",
          "lineNumber": 14,
          "columnNumber": 39
        },
        {
          "functionName": "invoke_static_method",
          "scriptId": "5",
          "url": "http://localhost:9400/debugger-driver.html",
          "lineNumber": 25,
          "columnNumber": 10
        },
        {
          "functionName": "",
          "scriptId": "47",
          "url": "",
          "lineNumber": 0,
          "columnNumber": 31
        }
      ]
    },
    "exception": {
      "type": "object",
      "className": "ExitStatus",
      "description": "ExitStatus",
      "objectId": "7004380125509995368.2.141",
      "preview": {
        "type": "object",
        "description": "ExitStatus",
        "overflow": false,
        "properties": [
          {
            "name": "name",
            "type": "string",
            "value": "ExitStatus"
          },
          {
            "name": "message",
            "type": "string",
            "value": "Program terminated with exit(0)"
          },
          {
            "name": "status",
            "type": "number",
            "value": "0"
          }
        ]
      }
    },
    "executionContextId": 2
  }
}

pavelsavara · 2021-05-06T11:24:49Z

We agreed with @radical that we would open another issue 52445 for 'req->refcount not met' problem as it reproduces even without the runtime changes on this PR.

I renamed files and methods to make it clear what is just preparation and what is actually being tested. I also added comments.

radical · 2021-05-06T21:57:39Z

I renamed files and methods to make it clear what is just preparation and what is actually being tested. I also added comments.

Sorry, I wasn't clear. I meant that these tests are not related to Stepping, but are about trying to read values for locals before they get initialized. And so, I was suggesting that we should rename the xunit test names to reflect that.

radical

Other than the comments, looks good. Thank you for testing on windows, and with Debug!

src/mono/wasm/debugger/DebuggerTestSuite/AssignmentTests.cs

src/mono/wasm/debugger/tests/Directory.Build.props

radical

Other than the cleanup comments, looks good 👍

radical · 2021-05-07T14:29:51Z

src/mono/wasm/debugger/DebuggerTestSuite/AssignmentTests.cs

+                locals_fn: (locals) =>
+                {
+                    Assert.Equal(2, locals.Count());
+                    Check(locals, "r", checkDefault);


Instead of the new overloads that use CheckValue, you can use:

await CheckProps(locals, new { r = checkDefault }, "default", num_fields: 2);

But since this needs to be async, you will need to do:

var pause_location = await StepAndCheck(StepKind.Into, "dotnet://debugger-test.dll/debugger-assignment-test.cs", -1, -1, "TestedMethod"); var locals = await GetProperties(pause_location["callFrames"][0]["callFrameId"].Value<string>()); await CheckProps(locals, new { r = checkDefault }, "default", num_fields: 2);

lewing

Looks good, we can do any more clean-up in followups

pavelsavara requested review from CoffeeFlux, imhameed, lambdageek, marek-safar, SamMonoRT and vargaz as code owners April 26, 2021 12:11

ghost added the area-Debugger-mono label Apr 26, 2021

pavelsavara changed the title ~~[wasm][debugger] - RuntimeError: memory access out of bounds~~ WIP: [wasm][debugger] - RuntimeError: memory access out of bounds Apr 26, 2021

lewing added the arch-wasm WebAssembly architecture label Apr 26, 2021

lewing marked this pull request as draft April 26, 2021 16:16

pavelsavara force-pushed the pr_wasm_debugger_crash branch from 3e927fa to 64a5a32 Compare April 30, 2021 15:29

pavelsavara marked this pull request as ready for review May 2, 2021 13:51

pavelsavara force-pushed the pr_wasm_debugger_crash branch from 64a5a32 to 50cb01c Compare May 2, 2021 19:41

pavelsavara mentioned this pull request May 3, 2021

[wasm][debugger] Don't publish the task #52172

Closed

pavelsavara changed the title ~~WIP: [wasm][debugger] - RuntimeError: memory access out of bounds~~ [wasm][debugger] - RuntimeError: memory access out of bounds May 3, 2021