Skip to content

[browser] dev tools profiler - method name resolution #115726

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

Merged
merged 2 commits into from
May 20, 2025

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented May 19, 2025

  • dereference of un-initialized buffer
  • snprintf doesn't zero-terminate too long strings

Fixes #115551

@pavelsavara pavelsavara added this to the 10.0.0 milestone May 19, 2025
@pavelsavara pavelsavara self-assigned this May 19, 2025
@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-Diagnostics-mono os-browser Browser variant of arch-wasm labels May 19, 2025
Copy link
Contributor

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

@pavelsavara pavelsavara marked this pull request as ready for review May 19, 2025 15:57
@Copilot Copilot AI review requested due to automatic review settings May 19, 2025 15:57
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an uninitialized buffer dereference and ensures overly long method names are handled correctly by zero-terminating snprintf. It also adds a stress test for long class names in the browser profiler and updates test configuration and scenario listings.

  • Introduce a long-named class in BrowserProfilerTest to validate profiler support for long symbols
  • Patch main.js to track performance.measure("TestMeaning") and adjust exit logic
  • Harden mono_wasm_method_get_name_ex in driver.c against null names and enforce zero-termination
  • Update diagnostics test setup and register it in the build scenarios list

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/mono/wasm/testassets/.../main.js Override performance.measure, track invocation, update exit code
src/mono/wasm/testassets/.../BrowserProfilerTest.cs Add ClassWithVeryVeryLongName… and instantiate in TestMeaning
src/mono/wasm/Wasm.Build.Tests/DiagnosticsTests.cs Switch to extraProperties, remove direct output assertion
src/mono/browser/runtime/driver.c Handle null method_name, zero-terminate snprintf buffer
eng/testing/scenarios/BuildWasmAppsJobsList.txt Add DiagnosticsTests to scenario list
Comments suppressed due to low confidence (4)

src/mono/wasm/testassets/WasmBasicTestApp/App/wwwroot/main.js:316

  • The override of performance.measure no longer calls the original origMeasure, which may break other performance measurements. Consider invoking origMeasure(method, options) within the override after tracking foundB.
globalThis.performance.measure = (method, options) => {

src/mono/browser/runtime/driver.c:526

  • [nitpick] The size 128 is a magic number. Define a named constant (e.g. MAX_METHOD_NAME_LEN) to clarify its purpose and make future adjustments easier.
res = (char *) malloc (128);

src/mono/wasm/Wasm.Build.Tests/DiagnosticsTests.cs:55

  • The test no longer asserts that performance.measure: TestMeaning was called, potentially missing regressions in profiler activation. Reintroduce an assertion on the browser output or exit code to ensure the profiler hook ran.
await RunForBuildWithDotnetRun(new BrowserRunOptions(Configuration: config, TestScenario: "BrowserProfilerTest"));

src/mono/wasm/testassets/WasmBasicTestApp/App/wwwroot/main.js:315

  • [nitpick] The variable foundB is not descriptive. Consider renaming it to foundTestMeaning or similar to clarify its purpose.
let foundB = false;

@pavelsavara pavelsavara changed the title [browser] dev tools profiler - long class name [browser] dev tools profiler - method name resolution May 19, 2025
res = (char *) malloc (128);
snprintf (res, 128,"%s.%s", mono_class_get_name (mono_method_get_class (method)), method_name);
res[127] = '\0';
Copy link
Member

Choose a reason for hiding this comment

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

this is harmless but snprintf should null terminate even at max length, is it really not?

Copy link
Member Author

Choose a reason for hiding this comment

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

MS Doc
The _snprintf family of functions only appends a terminating NULL character if the formatted string length is strictly less than count characters.

Maybe emscripten libc is different, but I wanted to be sure.

@pavelsavara pavelsavara merged commit b6a8764 into dotnet:main May 20, 2025
35 checks passed
@pavelsavara pavelsavara deleted the out_of_bounds branch May 20, 2025 06:37
@github-actions github-actions bot locked and limited conversation to collaborators Jun 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Diagnostics-mono os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught (in promise) RuntimeError: memory access out of bounds
3 participants